dgoldman marked 2 inline comments as done.
dgoldman added inline comments.
================
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);
----------------
sammccall wrote:
> 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.
> 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.
Thanks, I'll look into that in a follow up - I'd imagine it could affect lots
of other things but I'm not sure.
> 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.
Interesting, I wonder how Xcode works with this (e.g. for refactorings), I'll
ask around to see if anyone has suggestions/further context here.
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
