[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 inline comments.


================
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:
> > 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?
> The main users of this are in name lookup proper (where we store the lookup results on the path to avoid redoing the class scope hash table lookup) and now in access checking (where we store the most-accessible declaration). I think there's a couple of other name-lookup-like clients that also store a lookup result here.
> 
> In all cases the side storage is just a convenience. But it's probably an important convenience; it's awkward for a client to maintain a mapping on the side, because the key for that map would end up being the entire base path. 
> 
> We could just number the paths as we find them, and let the consumers build a `SmallVector` on the side rather than storing data in the `CXXBasePath`s. We'd need to be careful about the post-processing pass in `lookupInBases` that removes virtual base paths that are shadowed by non-virtual inheritance from the vbase, but that seems feasible. Worthwhile?
I see what you're saying.  We have places that compute all the base paths in an array, and it's convenient to be able to stash things into the elements of that array instead of having a second data structure that's parallel to it.  I guess there are three options:

- Tell clients to just make a parallel structure.

- Have this sort of intrusive storage and just accept that clients that want to use it will have some awkward casting to do.

- Get rid of the initial array dependence by making it a callback API instead of an array-building API, and then clients that want to build an array can build an array with the type they actually want.

I guess the third might be awkward?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94987