sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:309-313
+ // FIXME: visiting this here allows us to hover on UIColor in
+ // `UIColor.blackColor` but then `blackColor` no longer refers to the
+ // method.
+ // if (OPRE->isClassReceiver())
+ // Outer.add(OPRE->getClassReceiver(), Flags);
----------------
dgoldman wrote:
> sammccall wrote:
> > dgoldman wrote:
> > > Is there a good way to handle this case where the expression can refer to
> > > multiple decls?
> > TL;DR: yes, fix the AST :-)
> >
> > The design assumption is that an AST node (specifically: the tokens owned
> > by exactly that AST node and not a descendant of it) refers to one thing. I
> > don't see an easy way to hack around this.
> >
> > Expressions like this are supposed to be split into multiple AST nodes. But
> > ObjC seems to be missing AST nodes in a bunch of places (Protocol lists is
> > another).
> >
> > e.g. in C++ equivalent DeclRefExpr `UIColor::blackColor` there'd be both a
> > NestedNameSpecifier for `UIColor::` and a TagTypeLoc for `UIColor`. And
> > objc `[Test foo]` has an `ObjCInterfaceTypeLoc` for `Test`.
> >
> > ---
> >
> > I'm not sure this is the right place for this comment, the problem isn't
> > what VisitObjCPropertyRefExpr is doing, the issue is that the caller is
> > passing the wrong kind of node to findTarget() because there's no right one.
> That seems a bit invasive - is it really "fixing" the AST - is there anything
> wrong with the current AST design or rather any notable improvements by
> adding more nodes to the AST beside this one use case here?
>
> It seems like the design could be adapted if we had a SourceLocation hint
> here (from the caller) to disambiguate which one we mean, but I'm guessing
> not all of the callers/users of this would have one?
> That seems a bit invasive
I'm not sure it actually requires any changes to AST classes, just to
RecursiveASTVisitor to wrap the getClassReceiverType() & getReceiverLocation()
into n ObjCInterfaceTypeLoc (which is a value object) and visit it.
> is it really "fixing" the AST
It's inconsistent with the design of the rest of the AST.
There are very few other cases where AST nodes have this kind of compound
meaning rather than being to split into sub-nodes.
(In fact ObjC protocol lists are the only example I can think of. I suspect
this dates back to early clang days, maybe before the patterns became
established)
And similarly, there are few/no places where types are referenced that don't
have a TypeLoc node traversed by RecursiveASTVisitor.
> is there anything wrong with the current AST design or rather any notable
> improvements by adding more nodes to the AST beside this one use case here?
I guess the use case here is "precisely communicating a selection in an IDE".
Expand-selection features and show-ast-structure features don't work in clangd
for a similar reason.
Outside clangd I'm less familiar of course! Matchers, clang-tidy checks and
refactorings spring to mind.
For instance, if you want to write a tool to rename/split an ObjC class, then
something like `typeLoc(loc(asString("FooClass")))` would match all usages...
except this one, so your tool would have a bug.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99975/new/
https://reviews.llvm.org/D99975
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
