[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
This revision was automatically updated to reflect the committed changes. Closed by commit rG512aa8485010: [clangd] Handle members of anon structs in SelectionTree (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 Files: clang-tools-extra/clangd/Selection.cpp clang-tools-extra/clangd/unittests/XRefsTests.cpp Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,15 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure implicit access of anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If Base is an implicit CXXThis, then the whole MemberExpr has no +// tokens. If it's a normal e.g. DeclRef, we treat the MemberExpr like +// an implicit cast. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,15 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure implicit access of anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If Base is an implicit CXXThis, then the whole MemberExpr has no +// tokens. If it's a normal e.g. DeclRef, we treat the MemberExpr like +// an implicit cast. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet updated this revision to Diff 376445. kadircet marked 2 inline comments as done. kadircet added a comment. - update comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 Files: clang-tools-extra/clangd/Selection.cpp clang-tools-extra/clangd/unittests/XRefsTests.cpp Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,15 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure implicit access of anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If Base is an implicit CXXThis, then the whole MemberExpr has no +// tokens. If it's a normal e.g. DeclRef, we treat the MemberExpr like +// an implicit cast. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,15 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure implicit access of anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If Base is an implicit CXXThis, then the whole MemberExpr has no +// tokens. If it's a normal e.g. DeclRef, we treat the MemberExpr like +// an implicit cast. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/Selection.cpp:446 return true; + // Make sure anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { hmm, "implicit accesses of anonymous structs"? Comment at: clang-tools-extra/clangd/Selection.cpp:450 + if (FD->isAnonymousStructOrUnion()) +// If the base is not implicit, it can own tokens, otherwise we should +// stop traversal. The double negatives and references to a "traversal" here seem confusing. Maybe something more concrete? ``` // If Base is an implicit CXXThis, then the whole MemberExpr has no tokens. // If it's a normal e.g. DeclRef, we treat the MemberExpr like an implicit cast. ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:443 // It would be nice if RAV handled this (!shouldTraverseImplicitCode()). if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) sammccall wrote: > kadircet wrote: > > sammccall wrote: > > > seems like it'd be more consistent to handle it here? > > > > > > If it's a MemberExpr and the member is in an anonymous struct, then it's > > > implicit if `isImplicit(ME.getBase())`. > > the test case was actually to demonstrate why we can't do it here. we want > > to still keep traversing the AST after hitting a field inside an anon > > struct. handling here terminates the traversal for that subtree completely, > > and the real node we are interested might be down the tree (`y.[[x]]` in > > the test case). > That's what I meant by `isImplicit(ME.getBase())` > > The following passes tests (added to isImplicit)... > ``` > if (auto *ME = llvm::dyn_cast(S)) > if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) > if (FD->isAnonymousStructOrUnion()) > return isImplicit(ME->getBase()); > ``` > > > and the member is in an anonymous struct > > Oops, I meant "and the member *is* an anonymous struct". > It seems more natural to recognize and special-case the field-access to the > anon struct itself (which is intuitively implicit!), than the one to a normal > field within it. right, it makes sense. i was missing the isImplicit(base) bit + got stunned by the beautiful shape of the AST. thanks for bearing with me :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet updated this revision to Diff 376238. kadircet marked 4 inline comments as done. kadircet added a comment. - Use `isImplicit` rather than `earlyClaim` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 Files: clang-tools-extra/clangd/Selection.cpp clang-tools-extra/clangd/unittests/XRefsTests.cpp Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,14 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If the base is not implicit, it can own tokens, otherwise we should +// stop traversal. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,27 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +struct $2[[Foo]] { + struct { int $1[[x]]; }; + void foo() { +// Make sure the implicit base is skipped. +$1^x = 42; + } +}; +// Check that we don't skip explicit bases. +int a = $2^Foo{}.x; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; + EXPECT_THAT( + locateSymbolAt(AST, Code.point("2"), TU.index().get()), + UnorderedElementsAre(Sym("Foo", Code.range("2"), Code.range("2"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -443,6 +443,14 @@ if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) return true; + // Make sure anonymous structs don't end up owning tokens. + if (auto *ME = llvm::dyn_cast(S)) { +if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) + if (FD->isAnonymousStructOrUnion()) +// If the base is not implicit, it can own tokens, otherwise we should +// stop traversal. +return isImplicit(ME->getBase()); + } // Refs to operator() and [] are (almost?) always implicit as part of calls. if (auto *DRE = llvm::dyn_cast(S)) { if (auto *FD = llvm::dyn_cast(DRE->getDecl())) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
sammccall added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:368 +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( kadircet wrote: > sammccall wrote: > > This example is quite complicated to the point where it's hard to see what > > it's testing... The example you have in the comment is much clearer. > > > > If we also want to test named fields with inline struct types, maybe that > > should be a second test... > it is actually important to demonstrate we are not claiming the whole range > at the first encounter (and to make sure traversal is not stopped for any > other reason). Makes sense, but without comment it's not clear. Can we make the latter a separate test case, and simplify it too? This seems to be enough: ``` struct [[Foo]] { struct { int x; } }; int a = ^Foo{}.x; ``` Here the Foo{} is the non-empty base of the anon member access that must be traversed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
sammccall added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:443 // It would be nice if RAV handled this (!shouldTraverseImplicitCode()). if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) kadircet wrote: > sammccall wrote: > > seems like it'd be more consistent to handle it here? > > > > If it's a MemberExpr and the member is in an anonymous struct, then it's > > implicit if `isImplicit(ME.getBase())`. > the test case was actually to demonstrate why we can't do it here. we want to > still keep traversing the AST after hitting a field inside an anon struct. > handling here terminates the traversal for that subtree completely, and the > real node we are interested might be down the tree (`y.[[x]]` in the test > case). That's what I meant by `isImplicit(ME.getBase())` The following passes tests (added to isImplicit)... ``` if (auto *ME = llvm::dyn_cast(S)) if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) if (FD->isAnonymousStructOrUnion()) return isImplicit(ME->getBase()); ``` > and the member is in an anonymous struct Oops, I meant "and the member *is* an anonymous struct". It seems more natural to recognize and special-case the field-access to the anon struct itself (which is intuitively implicit!), than the one to a normal field within it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:443 // It would be nice if RAV handled this (!shouldTraverseImplicitCode()). if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) sammccall wrote: > seems like it'd be more consistent to handle it here? > > If it's a MemberExpr and the member is in an anonymous struct, then it's > implicit if `isImplicit(ME.getBase())`. the test case was actually to demonstrate why we can't do it here. we want to still keep traversing the AST after hitting a field inside an anon struct. handling here terminates the traversal for that subtree completely, and the real node we are interested might be down the tree (`y.[[x]]` in the test case). Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:368 +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( sammccall wrote: > This example is quite complicated to the point where it's hard to see what > it's testing... The example you have in the comment is much clearer. > > If we also want to test named fields with inline struct types, maybe that > should be a second test... it is actually important to demonstrate we are not claiming the whole range at the first encounter (and to make sure traversal is not stopped for any other reason). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
sammccall added a comment. Thanks for fixing! Early-claim is a blunt hammer and I'm always dreading side-effects. This also feels a *lot* like implicit-this which is handled in another way. So that seems worth a shot, but this approach is fine if that one doesn't work. Comment at: clang-tools-extra/clangd/Selection.cpp:443 // It would be nice if RAV handled this (!shouldTraverseImplicitCode()). if (auto *CTI = llvm::dyn_cast(S)) if (CTI->isImplicit()) seems like it'd be more consistent to handle it here? If it's a MemberExpr and the member is in an anonymous struct, then it's implicit if `isImplicit(ME.getBase())`. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:368 +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( This example is quite complicated to the point where it's hard to see what it's testing... The example you have in the comment is much clearer. If we also want to test named fields with inline struct types, maybe that should be a second test... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:745 + if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) { +if (FD->getParent()->isAnonymousStructOrUnion()) + return SourceRange(ME->getMemberLoc(), ME->getEndLoc()); in theory this check should be redundant, as each field should own its tokens. but given the complexity of C++ just wanted to be conservative. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110825/new/ https://reviews.llvm.org/D110825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110825: [clangd] Handle members of anon structs in SelectionTree
kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: usaxena95, arphaman. kadircet requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. References to fields inside anon structs contain an implicit children for the container, which has the same SourceLocation with the field. This was resulting in SelectionTree always picking the anon-struct rather than the field as the selection. This patch prevents that by claiming the range for the field early. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D110825 Files: clang-tools-extra/clangd/Selection.cpp clang-tools-extra/clangd/unittests/XRefsTests.cpp Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,25 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +class Foo { + struct { +struct { + int $1[[x]]; +} $0[[y]]; + }; + void foo() { $0^y.$1^x = 2; } +}; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("0"), TU.index().get()), + UnorderedElementsAre(Sym("y", Code.range("0"), Code.range("0"; + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -732,6 +732,19 @@ } else if (const auto *CCI = N.get()) { // : [[b_]](42) return CCI->getMemberLocation(); +} else if (const auto *ME = N.get()) { + // In member expressions involving anonymous structs, there's an implicit + // reference to the container. Make sure all the tokens that belong to the + // member are claimed first to prevent this unspelled children from owning + // them. + // struct Foo { + // struct { int x; } + // void foo() { [[x]] = 2; } + // }; + if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) { +if (FD->getParent()->isAnonymousStructOrUnion()) + return SourceRange(ME->getMemberLoc(), ME->getEndLoc()); + } } return SourceRange(); } Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp === --- clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -365,6 +365,25 @@ ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range(; } +TEST(LocateSymbol, AnonymousStructFields) { + auto Code = Annotations(R"cpp( +class Foo { + struct { +struct { + int $1[[x]]; +} $0[[y]]; + }; + void foo() { $0^y.$1^x = 2; } +}; + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point("0"), TU.index().get()), + UnorderedElementsAre(Sym("y", Code.range("0"), Code.range("0"; + EXPECT_THAT(locateSymbolAt(AST, Code.point("1"), TU.index().get()), + UnorderedElementsAre(Sym("x", Code.range("1"), Code.range("1"; +} + TEST(LocateSymbol, FindOverrides) { auto Code = Annotations(R"cpp( class Foo { Index: clang-tools-extra/clangd/Selection.cpp === --- clang-tools-extra/clangd/Selection.cpp +++ clang-tools-extra/clangd/Selection.cpp @@ -732,6 +732,19 @@ } else if (const auto *CCI = N.get()) { // : [[b_]](42) return CCI->getMemberLocation(); +} else if (const auto *ME = N.get()) { + // In member expressions involving anonymous structs, there's an implicit + // reference to the container. Make sure all the tokens that belong to the + // member are claimed first to prevent this unspelled children from owning + // them. + // struct Foo { + // struct { int x; } + // void foo() { [[x]] = 2; } + // }; + if (auto *FD = llvm::dyn_cast(ME->getMemberDecl())) { +if (FD->getParent()->isAnonymousStructOrUnion()) + return SourceRange(ME->getMemberLoc(), ME->getEndLoc()); + } } return SourceRange(); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits