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:
> > > > 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.
Okay. In the end, architecturally it's a little suspect that this is here, but
I definitely don't think we need to hold up this patch to eliminate it; we can
just flag that this is for arbitrary client use.
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
