rsmith 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 {
----------------
rjmccall wrote:
> 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?
Yes, the problem with the third option is its interaction with the
post-processing step in `CXXRecordDecl::lookupInBases`. We're not supposed to
consider virtual bases if they're shadowed along any path. The way that
`lookupInBases` handles this is by first finding all paths to subobjects, and
then removing any path containing an edge to a virtual base for which some
other path ends in a class deriving from that vbase. So we end up with the
callback being called spuriously in some cases, and `lookupInBases` ends up
producing only a subset of the paths that were followed.
I'm pretty sure we could rearchitect the lookup process so that this doesn't
happen -- so that we never visit a virtual base until we've finished traversing
all subobjects that virtually inherit from it -- but that's not what the
`lookupInBases` algorithm is doing right now.
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
