sammccall added a comment.
Thanks! I'd been meaning to do this before my vacation (and had a draft I'll
dig up in case there were other issues).
Generally I think we should be willing to change behavior in some marginal
cases, and I do think it's important to care about the layering of seltree vs
targetDecl because we'll want to plug them into other things in the future.
(E.g. using targetDecl + recursiveASTVisitor for more flexible indexing)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:139
+// that constructor. FIXME(nridge): this should probably be handled in
+// targetDecl() itself.
+const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) {
----------------
nridge wrote:
> I would appreciate some tips on how we could handle this in `targetDecl()`.
> Please see more specific comments below.
My thinking is basically:
- C++ syntax is vague and doesn't give us great ways of referring directly to
constructors.
- there's value to simplicity, and I've found go-to-definition additionally
finding implicit nodes to be confusing as often as useful. I think on balance
and given the constraints of LSP, we should consider dropping this and having
implicit references be returned by find-refs but not targetable by
go-to-def/hover/etc. It's OK for simplicity of implementation to be a concern
here.
- the node that targets the constructor is the CXXConstructExpr. Treating the
VarDecl conditionally as a reference to the constructor, while clever, seems
like a hack. Ideally the fix is to make SelectionTree yield the
CXXConstructExpr, not to change TargetDecl.
- CXXConstructExpr is only sometimes implicit. SelectionTree should return it
for the parens in `Foo()` or `Foo x()`. And ideally for the equals in `Foo x =
{}`, though I think there's no CXXConstructExpr in the AST in that case :-(
- When the AST modelling is bad (as it seems to be for some aspects
CXXConstructExpr) we should consider accepting some glitches and trying to
improve things in clang if they're important. Hacking around them will lead to
inconsistencies between different clangd features.
The TL;DR is I think I'd be happy to drop this special case and try to make
SelectionTree surface CXXConstructExpr more often, even if it changes some
behavior.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:147
}
+ while ((N = N->Parent)) {
+ if (N->ASTNode.get<CallExpr>() || N->ASTNode.get<ImplicitCastExpr>() ||
----------------
nridge wrote:
> This part is a super hacky way to keep case #9 in `LocateSymbol.Ambiguous`
> passing while not regressing other tests.
>
> In that case, the selection tree looks like this:
>
> ```
> DeclStmt Foo foox = Foo("asdf");
>
> VarDecl Foo foox = Foo("asdf")
> ExprWithCleanups Foo("asdf")
> CXXConstructExpr Foo("asdf")
> MaterializeTemporaryExpr Foo("asdf")
> CXXFunctionalCastExpr Foo("asdf")
> .RecordTypeLoc Foo
> ```
>
> The common ancestor is the `RecordTypeLoc`, but we'd like to find the
> constructor referred to by the `CXXConstructExpr` as a target as well. To
> achieve that, my current code walks up the parent tree until we encounter a
> `CXXConstructExpr`, but aborts the walk if certain node types are encountered
> to avoid certain other scenarios where we have a `CXXConstructExpr` ancestor.
>
> This is almost certainly wrong, as there are likely many other cases where we
> have a `CXXConstructExpr` ancestor but don't want to find the constructor as
> a target which are not being handled correctly.
>
> Is there a better way to identify the syntactic form at issue in this
> testcase?
This case is gross, but as above I think it would be OK to have the targeting
work like:
```
Foo("asdf")
FFFCSSSSSSC
F -> RecordTypeLoc -> CXXRecordDecl Foo
C -> CXXConstructExpr -> CXXConstructorDecl Foo(string)
S -> StringLiteral -> nullptr
```
WDYT?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:159
+getDeclAtPosition(ParsedAST &AST, SourceLocation Pos,
+ DeclRelationSet Relations = DeclRelation::TemplatePattern |
+ DeclRelation::Alias) {
----------------
I'd prefer not to privilege any defaults here, I think this is unfortunately
something that each caller needs to think about.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:864
}
- if (!HI && hasDeducedType(AST, SourceLocationBeg)) {
+ // Check for hasDeducedType() even if getDeclAtPosition() found something.
+ // This is needed because it will find something for "operator auto",
----------------
might be clearer to handle that case first then, and guard each with `if (!HI)`
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2194
const char *DeducedType;
- } Tests[] = {
- {"^auto i = 0;", "int"},
- {"^auto f(){ return 1;};", "int"}
- };
+ } Tests[] = {{"^auto i = 0;", "int"}, {"^auto f(){ return 1;};", "int"}};
for (Test T : Tests) {
----------------
nit: can we add the trailing comma in the init-list to preserve the older,
clearer formatting instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69237/new/
https://reviews.llvm.org/D69237
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
