rsmith added a comment.
In D94987#2507481 <https://reviews.llvm.org/D94987#2507481>, @rjmccall wrote:
> How does this new rule work if we find overlapping but non-equal sets of
> declarations in different subobjects? I'm sure you can get that with `using`
> declarations.
You can. The rule is that you resolve using-declarations to the declarations
they name, and you resolve typedef declarations to the types they name, and the
resulting set of declarations and types must be the same in every subobject in
which there are (non-shadowed) results -- if not, the lookup is ill-formed.
(Put another way, the set of entities found in each subobject must be the same,
but the set of declarations found in each subobject can differ.)
================
Comment at: clang/include/clang/AST/CXXInheritance.h:77
- CXXBasePath() = default;
+ /// Additional data stashed on the base path by its consumers.
+ union {
----------------
rjmccall wrote:
> Is there a way to know which case we're in, or do different consumers do
> different things?
We could use a discriminated union here. Ultimately, I think what we really
want is to provide some opaque storage for the consumer to use, rather than to
have some hard-coded list here. I haven't thought of a good way to expose that;
I don't think it's worth templating the entire base path finding mechanism to
add such storage, and a single pointer isn't large enough for a
`DeclContext::lookup_result`.
Vassil's https://reviews.llvm.org/D91524 will make the `lookup_result` case fit
into a single pointer; once we adopt that, perhaps we can switch to holding an
opaque `void*` here?
================
Comment at: clang/lib/Sema/SemaAccess.cpp:891
+ // Note that declaresSameEntity doesn't correctly determine whether
+ // two type declarations declare the same type entity.
+ if ((!Best || Best->getAccess() > Corresponding->getAccess()) &&
----------------
rjmccall wrote:
> Is it okay to check them in sequence like this, or do we need to check in
> advance which case to use?
For types, `declaresSameEntity` returns true for a subset of the cases for
which `declaresSameType` returns true, so it will always be correct to check
both (though for a type, checking `declaresSameEntity` will always be
redundant).
================
Comment at: clang/lib/Sema/SemaAccess.cpp:983
+ }
+ return false;
+ };
----------------
rjmccall wrote:
> Is this not doing a *lot* of extra work? I suppose this is only in the slow
> path where we weren't able to immediately recognize that the found decl is
> public or we're in a scope with obviously adequate access?
Compared to the `isDerivedFrom` check, this will be doing one extra decl
context hash table lookup per class we consider on each path. I haven't
measured the extra work here; I can if you're concerned.
I thought for a while about better ways to handle this, and I'm not sure
there's a good middle ground between redoing the work like this and saving the
base paths from name lookup and using them here. If you have better ideas, I'd
appreciate them!
I suppose we probably have a spare bit on `DeclAccessPair` that we could use to
track whether we're in the "hard" (found in classes of different types) case,
and we could use `isDerivedFrom` in the "easy" cases. Do you think that's
worthwhile?
Ah, I see you suggest doing something like that below. Yes, I can give that a
go.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94987/new/
https://reviews.llvm.org/D94987
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
