[PATCH] D94987: DR39: Perform ambiguous subobject checks for class member access as part of object argument conversion, not as part of name lookup.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> 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.)

Okay, makes sense.  So the DeclAccessPairs in the lookup result are basically just the first+best pair we found for any particular entity?



================
Comment at: clang/include/clang/AST/CXXInheritance.h:77
 
-  CXXBasePath() = default;
+  /// Additional data stashed on the base path by its consumers.
+  union {
----------------
rsmith wrote:
> 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?
Yeah, that might be cleaner, assuming we really need this.  What are the clients that need to store something specifically in the `CXXBasePath` object and can't just keep it separate?


================
Comment at: clang/lib/Sema/SemaAccess.cpp:983
+      }
+      return false;
+    };
----------------
rsmith wrote:
> 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.
Thanks.  Yeah, I think it would be nice if the very common case of finding something in a unique class didn't have to redo any lookups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94987/new/

https://reviews.llvm.org/D94987