Since the heuristic resolver is created in a place where the
ASTContext is available, it can store the ASTContext and the
NameFactory hack can be removed.
The patch also renames getMembersReferencedViaDependentName()
to resolveDependentMember().
Differential D92290
[clangd] Factor out the heuristic resolver code into its own class nridge on Nov 29 2020, 3:06 PM. Authored by
Details Since the heuristic resolver is created in a place where the The patch also renames getMembersReferencedViaDependentName()
Diff Detail
Event TimelineComment Actions Sorry for taking a while to get to this... I'm not sure this is really a helpful use of Context. (Though some would argue that there are no good uses, my manager was appalled when we added it...) There's of course no hard-and-fast rules but my checklist is:
Here it's a mandatory input, and the alternative is cluttering a couple of layers of purely-private interfaces, so this seems like a lot of magic to avoid a fairly small amount of awkwardness. (The original use cases for context were around VFS state for embedders, where *all* these things were true - the lspEncoding() is another place where this is IMO a pretty good tradeoff) Comment Actions :-)
I was inspired to do this by a work-in-progress patch I have to fix https://github.com/clangd/clangd/issues/451, for which I need to make Sema available in a similar way. That patch isn't quite ready for review (there are some crashes / assertion failures that I need to iron out), but I posted what I have so far at D93522 if you're curious.
I agree that there is no fundamental need to use Context, it just seemed convenient (and hey, ASTContext even has Context in its name :-)). I could just pass the ASTContext as a parameter to targetDecl() and other relevant functions. Perhaps, with a view to eventually needing Sema as well, I should just pass ParsedAST instead? Comment Actions Ah, I suspected something like this was afoot! The patch looks... exciting :-) And I definitely agree we should remove the hacks if we're going to have access to ASTContext anyway. I think an explicit parameter here is probably a good idea, it's a bit sad to lose targetDecl's simple signature but seems justified at this point. targetDecl(DynTypedNode N, HeuristicResolver * = nullptr) The HeuristicResolver object could provide primitives like finding the pointee type, template patterns etc. It would hold a reference to Sema/ASTContext. There are a few reasons for this:
What do you think about this? Comment Actions That all sounds reasonable -- thanks for the suggestions! I've been meaning to factor out the heuristic resolution stuff into its own file anyways, this patch is as good a time as any to do it. Comment Actions Change approach to factor out a HeuristicResolver class I didn't make it an optional parameter to allTargetDecls() etc. If you feel there are call sites which would benefit from not passing one in, we can change this. Comment Actions Thanks a lot for doing this! I'm not sure how we should sequence that/how much to do. I would like to get your feedback on the ideas though as I spent a *long* time staring at this code today and want to make sure I'm not crazy :-) As far as I know we should always have this available in production, so I don't think it needs to be optional in the signature.
Comment Actions Thanks for the comments! I'll have a more detailed look later, but just wanted to ask one clarifying question for now:
Comment Actions Thanks for the thoughtful comments about the API surface and such! I think I've addressed or responded to most of them. There are a few outstanding comments about how the HeuristicResolver object is stored and passed around (const, pointer, etc.), which I'll come back to once we come to a resolution on the nullability question. Comment Actions Sorry for the looong delay getting back to this :-\ I do think this should be nullable (and const-friendly) but otherwise this is good to land.
Comment Actions Ok, I've made HeuristicResolver nullable, in the sense that I've added null checks arounds its use in TargetFinder. I haven't refactored the tests to pass in a null resolver / annotate the ones that do or don't need one. Would you like that done in this patch, or a follow-up?
Comment Actions Thanks so much for pushing this through, it looks great. Let's go ahead and land this, I'm happy to make the test changes.
|
only Identifier needs the heuristic resolution, TypeSpec[WithTemplate] can just pull out the stored type