[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd92413a45e20: [clangd] Selection handles CXXBaseSpecifier
(authored by njames93).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang/lib/AST/ASTTypeTraits.cpp
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+struct B : priv^ate A {};
+ )cpp",
+ "Cannot rename symbol: there is no symbol at the given location", false,
+ nullptr},
};
for (const auto& Case : Cases) {
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,9 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+return traverseNode(, [&] { return Base::TraverseCXXBaseSpecifier(X); });
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+struct B : priv^ate A {};
+ )cpp",
+ "Cannot rename symbol: there is no symbol at the given
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 updated this revision to Diff 318933.
njames93 marked an inline comment as done.
njames93 added a comment.
Remove FindTarget to instead add in a follow up patch.
Side note, Think I'll wait til after tomorrow to land this, unsure if the
changes will break anything elsewhere.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang/lib/AST/ASTTypeTraits.cpp
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+struct B : priv^ate A {};
+ )cpp",
+ "Cannot rename symbol: there is no symbol at the given location", false,
+ nullptr},
};
for (const auto& Case : Cases) {
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,9 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+return traverseNode(, [&] { return Base::TraverseCXXBaseSpecifier(X); });
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 marked 2 inline comments as done.
njames93 added inline comments.
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
sammccall wrote:
> njames93 wrote:
> > sammccall wrote:
> > > njames93 wrote:
> > > > Is this good behaviour, or should Foo selection resolve as a
> > > > RecordTypeLoc, That is the behaviour previously observed for these 2
> > > > cases. And instead have CXXBaseSpecifier as its parent selection?
> > > Yes, this should be a typeloc with the CXXBaseSpecifier as its parent,
> > > unless there's a really compelling reason to make an exception.
> > >
> > > Two main reasons:
> > > - it matches RecursiveASTVisitor's idea of the tree shape, which people
> > > mostly understand. If we diverge, it'll be a special case we need to
> > > remember and so potentially a source of bugs.
> > > - This allows code that handles types generically to work with fewer
> > > special cases.
> > > (I do like the FindTarget change though - it seems reasonable to allow
> > > go-to-def on e.g. the `public` keyword)
> > > It still preserves flexibility if we want to handle types as
> > > base-specifiers specially (we can examine the parent)
> > OK, funnily enough that change to find target was to fix renaming when the
> > recordtypeloc was removed from the selection. That change will be redundant
> > if i fix that, but do you still want it in here?
> Up to you, but I'd lean towards keeping it and adding a FindTarget test for
> `struct S : ^private T` or so, which is the visible change.
I think I should remove it, then add it back in a follow up. It was needed to
fix a issue in here that was then resolved in here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
(I don't see a new snapshot but LG with the traverseNode change)
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
njames93 wrote:
> sammccall wrote:
> > njames93 wrote:
> > > Is this good behaviour, or should Foo selection resolve as a
> > > RecordTypeLoc, That is the behaviour previously observed for these 2
> > > cases. And instead have CXXBaseSpecifier as its parent selection?
> > Yes, this should be a typeloc with the CXXBaseSpecifier as its parent,
> > unless there's a really compelling reason to make an exception.
> >
> > Two main reasons:
> > - it matches RecursiveASTVisitor's idea of the tree shape, which people
> > mostly understand. If we diverge, it'll be a special case we need to
> > remember and so potentially a source of bugs.
> > - This allows code that handles types generically to work with fewer
> > special cases.
> > (I do like the FindTarget change though - it seems reasonable to allow
> > go-to-def on e.g. the `public` keyword)
> > It still preserves flexibility if we want to handle types as
> > base-specifiers specially (we can examine the parent)
> OK, funnily enough that change to find target was to fix renaming when the
> recordtypeloc was removed from the selection. That change will be redundant
> if i fix that, but do you still want it in here?
Up to you, but I'd lean towards keeping it and adding a FindTarget test for
`struct S : ^private T` or so, which is the visible change.
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
njames93 wrote:
> sammccall wrote:
> > nice, thanks!
> > We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a
> > simple pattern to follow for sourcerange)
> Not really trivial as there is no matcher for `cxxBaseSpecifier`. Annoyingly
> its my fault there isn't one.
Oh, I've been down this rabbithole recently... clangd support for attrs ->
DynTypedNode support for Attr -> matchers for use in tests -> matchers get
complicated...
Let's not block on this (and I need to revive my attr patch that got too big...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 updated this revision to Diff 318930.
njames93 marked 2 inline comments as done.
njames93 added a comment.
Update behaviour to keep RecordTypeLoc seletion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
Files:
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang/lib/AST/ASTTypeTraits.cpp
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+struct B : priv^ate A {};
+ )cpp",
+ "Cannot rename symbol: there is no symbol at the given location", false,
+ nullptr},
};
for (const auto& Case : Cases) {
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,9 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+return traverseNode(, [&] { return Base::TraverseCXXBaseSpecifier(X); });
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
Index: clang-tools-extra/clangd/FindTarget.cpp
===
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -710,7 +710,8 @@
Finder.add(CCI, Flags);
else if (const TemplateArgumentLoc *TAL = N.get())
Finder.add(TAL->getArgument(), Flags);
-
+ else if (const CXXBaseSpecifier *CBS = N.get())
+Finder.add(CBS->getType(), Flags);
return Finder.takeDecls();
}
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : private [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "RecordTypeLoc",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index:
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 marked 2 inline comments as done.
njames93 added inline comments.
Comment at: clang-tools-extra/clangd/Selection.cpp:496
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+auto N = DynTypedNode::create(X);
sammccall wrote:
> can we not use traverseNode here, rather than repeating?
Yeah, especially now I'm changing it to actually traverse :)
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
sammccall wrote:
> njames93 wrote:
> > Is this good behaviour, or should Foo selection resolve as a RecordTypeLoc,
> > That is the behaviour previously observed for these 2 cases. And instead
> > have CXXBaseSpecifier as its parent selection?
> Yes, this should be a typeloc with the CXXBaseSpecifier as its parent, unless
> there's a really compelling reason to make an exception.
>
> Two main reasons:
> - it matches RecursiveASTVisitor's idea of the tree shape, which people
> mostly understand. If we diverge, it'll be a special case we need to remember
> and so potentially a source of bugs.
> - This allows code that handles types generically to work with fewer special
> cases.
> (I do like the FindTarget change though - it seems reasonable to allow
> go-to-def on e.g. the `public` keyword)
> It still preserves flexibility if we want to handle types as
> base-specifiers specially (we can examine the parent)
OK, funnily enough that change to find target was to fix renaming when the
recordtypeloc was removed from the selection. That change will be redundant if
i fix that, but do you still want it in here?
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
sammccall wrote:
> nice, thanks!
> We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a
> simple pattern to follow for sourcerange)
Not really trivial as there is no matcher for `cxxBaseSpecifier`. Annoyingly
its my fault there isn't one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/Selection.cpp:496
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+auto N = DynTypedNode::create(X);
can we not use traverseNode here, rather than repeating?
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
njames93 wrote:
> Is this good behaviour, or should Foo selection resolve as a RecordTypeLoc,
> That is the behaviour previously observed for these 2 cases. And instead have
> CXXBaseSpecifier as its parent selection?
Yes, this should be a typeloc with the CXXBaseSpecifier as its parent, unless
there's a really compelling reason to make an exception.
Two main reasons:
- it matches RecursiveASTVisitor's idea of the tree shape, which people mostly
understand. If we diverge, it'll be a special case we need to remember and so
potentially a source of bugs.
- This allows code that handles types generically to work with fewer special
cases.
(I do like the FindTarget change though - it seems reasonable to allow
go-to-def on e.g. the `public` keyword)
It still preserves flexibility if we want to handle types as base-specifiers
specially (we can examine the parent)
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
nice, thanks!
We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a
simple pattern to follow for sourcerange)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 added inline comments.
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
Is this good behaviour, or should Foo selection resolve as a RecordTypeLoc,
That is the behaviour previously observed for these 2 cases. And instead have
CXXBaseSpecifier as its parent selection?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 updated this revision to Diff 318523.
njames93 added a comment.
Update FindTargets to fix symbol renaming on a CXXBaseSpecifier.
Add test to ensure rename wont trigger on a CXXBaseSpecifier.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95231/new/
https://reviews.llvm.org/D95231
Files:
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang/lib/AST/ASTTypeTraits.cpp
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1024,6 +1024,12 @@
}
)cpp",
"new name is the same", !HeaderFile, nullptr, "SameName"},
+ {R"cpp(// Ensure it doesn't associate base specifier with base name.
+struct A {};
+struct B : priv^ate A {};
+ )cpp",
+ "Cannot rename symbol: there is no symbol at the given location", false,
+ nullptr},
};
for (const auto& Case : Cases) {
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,14 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+auto N = DynTypedNode::create(X);
+if (canSafelySkipNode(N))
+ return true;
+push(std::move(N));
+pop();
+return true;
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
Index: clang-tools-extra/clangd/FindTarget.cpp
===
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -710,7 +710,8 @@
Finder.add(CCI, Flags);
else if (const TemplateArgumentLoc *TAL = N.get())
Finder.add(TAL->getArgument(), Flags);
-
+ else if (const CXXBaseSpecifier *CBS = N.get())
+Finder.add(CBS->getType(), Flags);
return Finder.takeDecls();
}
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
// Point
[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier
njames93 created this revision.
njames93 added reviewers: kadircet, hokein, sammccall.
Herald added subscribers: usaxena95, arphaman.
njames93 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
Selection now includes the virtual and access modifier as part of their range
for cxx base specifiers.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95231
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang/lib/AST/ASTTypeTraits.cpp
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,14 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+auto N = DynTypedNode::create(X);
+if (canSafelySkipNode(N))
+ return true;
+push(std::move(N));
+pop();
+return true;
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
Index: clang/lib/AST/ASTTypeTraits.cpp
===
--- clang/lib/AST/ASTTypeTraits.cpp
+++ clang/lib/AST/ASTTypeTraits.cpp
@@ -193,5 +193,7 @@
return TAL->getSourceRange();
if (const auto *C = get())
return SourceRange(C->getBeginLoc(), C->getEndLoc());
+ if (const auto *CBS = get())
+return CBS->getSourceRange();
return SourceRange();
}
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,27 @@
)cpp",
"StringLiteral", // Not DeclRefExpr to operator()!
},
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[v^ir^tual private Foo]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[private Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
+ {
+ R"cpp(
+struct Foo {};
+struct Bar : [[Fo^o]] {};
+ )cpp",
+ "CXXBaseSpecifier",
+ },
// Point selections.
{"void foo() { [[^foo]](); }", "DeclRefExpr"},
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -493,6 +493,14 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(X); });
}
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier ) {
+auto N = DynTypedNode::create(X);
+if (canSafelySkipNode(N))
+ return true;
+push(std::move(N));
+pop();
+return true;
+ }
// Stmt is the same, but this form allows the data recursion optimization.
bool dataTraverseStmtPre(Stmt *X) {
if (!X || isImplicit(X))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
