avogelsgesang created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
So far `insertDecl`, did not support insertion of new code after the
last top-level declaration in a file. This was, because a
`TranslationUnitDecl` does not have a valid `getEndLoc()`.
Also, the logic of `Anchor::Below` was somewhat surprising: I would have
expected `Below` to insert below the first match. Instead it insert
after the first contigous run of matches. I found this highly
unintuitive, and replaced it by an additional optional `First`/`Last`
flag which can be specified for an anchor.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D122101
Files:
clang-tools-extra/clangd/refactor/InsertionPoint.cpp
clang-tools-extra/clangd/refactor/InsertionPoint.h
clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
Index: clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
+++ clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
@@ -29,7 +29,7 @@
$b^// leading comment
int b;
$c^int c1; // trailing comment
- int c2;
+ $c2^int c2;
$a2^int a2;
$end^};
)cpp");
@@ -47,18 +47,24 @@
auto &NS = cast<NamespaceDecl>(findDecl(AST, "ns"));
// Test single anchors.
- auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction) {
- auto Loc = insertionPoint(NS, {Anchor{StartsWith(Prefix), Direction}});
+ auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction,
+ Anchor::SearchDir SearchDirection = Anchor::First) {
+ auto Loc = insertionPoint(
+ NS, {Anchor{StartsWith(Prefix), Direction, SearchDirection}});
return sourceLocToPosition(AST.getSourceManager(), Loc);
};
EXPECT_EQ(Point("a", Anchor::Above), Code.point("a"));
EXPECT_EQ(Point("a", Anchor::Below), Code.point("b"));
EXPECT_EQ(Point("b", Anchor::Above), Code.point("b"));
EXPECT_EQ(Point("b", Anchor::Below), Code.point("c"));
- EXPECT_EQ(Point("c", Anchor::Above), Code.point("c"));
- EXPECT_EQ(Point("c", Anchor::Below), Code.point("a2"));
+ EXPECT_EQ(Point("c", Anchor::Above, Anchor::First), Code.point("c"));
+ EXPECT_EQ(Point("c", Anchor::Below, Anchor::First), Code.point("c2"));
+ EXPECT_EQ(Point("c", Anchor::Above, Anchor::Last), Code.point("c2"));
+ EXPECT_EQ(Point("c", Anchor::Below, Anchor::Last), Code.point("a2"));
+ EXPECT_EQ(Point("a2", Anchor::Above), Code.point("a2"));
+ EXPECT_EQ(Point("a2", Anchor::Below), Code.point("end"));
EXPECT_EQ(Point("", Anchor::Above), Code.point("a"));
- EXPECT_EQ(Point("", Anchor::Below), Code.point("end"));
+ EXPECT_EQ(Point("", Anchor::Below), Code.point("b"));
EXPECT_EQ(Point("no_match", Anchor::Below), Position{});
// Test anchor chaining.
@@ -83,6 +89,35 @@
Code.point("end"));
}
+TEST(InsertionPointTests, TopLevel) {
+ Annotations Code(R"cpp(
+ $a^int a;
+ $b^int b;
+ $end^)cpp");
+
+ auto HasName =
+ [&](llvm::StringLiteral S) -> std::function<bool(const Decl *)> {
+ return [S](const Decl *D) {
+ if (const auto *ND = llvm::dyn_cast<NamedDecl>(D))
+ return ND->getNameAsString() == S;
+ return false;
+ };
+ };
+
+ auto AST = TestTU::withCode(Code.code()).build();
+ auto &TUD = *AST.getASTContext().getTranslationUnitDecl();
+
+ // Test single anchors.
+ auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction) {
+ auto Loc = insertionPoint(TUD, {Anchor{HasName(Prefix), Direction}});
+ return sourceLocToPosition(AST.getSourceManager(), Loc);
+ };
+ EXPECT_EQ(Point("a", Anchor::Above), Code.point("a"));
+ EXPECT_EQ(Point("a", Anchor::Below), Code.point("b"));
+ EXPECT_EQ(Point("b", Anchor::Above), Code.point("b"));
+ EXPECT_EQ(Point("b", Anchor::Below), Code.point("end"));
+}
+
// For CXX, we should check:
// - special handling for access specifiers
// - unwrapping of template decls
@@ -96,7 +131,7 @@
$private^private:
$field^int PrivField;
$method^void privMethod();
- template <typename T> void privTemplateMethod();
+ $template^template <typename T> void privTemplateMethod();
$end^};
)cpp");
@@ -114,11 +149,12 @@
EXPECT_EQ(Point({IsMethod, Anchor::Above}, AS_public), Code.point("Method"));
EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_public), Code.point("Field"));
EXPECT_EQ(Point({Any, Anchor::Above}, AS_public), Code.point("Method"));
- EXPECT_EQ(Point({Any, Anchor::Below}, AS_public), Code.point("private"));
+ EXPECT_EQ(Point({Any, Anchor::Below}, AS_public), Code.point("Method"));
EXPECT_EQ(Point({IsMethod, Anchor::Above}, AS_private), Code.point("method"));
- EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_private), Code.point("end"));
+ EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_private),
+ Code.point("template"));
EXPECT_EQ(Point({Any, Anchor::Above}, AS_private), Code.point("field"));
- EXPECT_EQ(Point({Any, Anchor::Below}, AS_private), Code.point("end"));
+ EXPECT_EQ(Point({Any, Anchor::Below}, AS_private), Code.point("field"));
EXPECT_EQ(Point({IsMethod, Anchor::Above}, AS_protected), Position{});
EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_protected), Position{});
EXPECT_EQ(Point({Any, Anchor::Above}, AS_protected), Position{});
Index: clang-tools-extra/clangd/refactor/InsertionPoint.h
===================================================================
--- clang-tools-extra/clangd/refactor/InsertionPoint.h
+++ clang-tools-extra/clangd/refactor/InsertionPoint.h
@@ -23,6 +23,8 @@
std::function<bool(const Decl *)> Match;
// Whether the insertion point should be before or after the matching block.
enum Dir { Above, Below } Direction = Below;
+ // Whether to choose the first or the last match
+ enum SearchDir { First, Last } SearchDirection = First;
};
// Returns the point to insert a declaration according to Anchors.
Index: clang-tools-extra/clangd/refactor/InsertionPoint.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/InsertionPoint.cpp
+++ clang-tools-extra/clangd/refactor/InsertionPoint.cpp
@@ -18,50 +18,6 @@
namespace clangd {
namespace {
-// Choose the decl to insert before, according to an anchor.
-// Nullptr means insert at end of DC.
-// None means no valid place to insert.
-llvm::Optional<const Decl *> insertionDecl(const DeclContext &DC,
- const Anchor &A) {
- bool LastMatched = false;
- bool ReturnNext = false;
- for (const auto *D : DC.decls()) {
- if (D->isImplicit())
- continue;
- if (ReturnNext)
- return D;
-
- const Decl *NonTemplate = D;
- if (auto *TD = llvm::dyn_cast<TemplateDecl>(D))
- NonTemplate = TD->getTemplatedDecl();
- bool Matches = A.Match(NonTemplate);
- dlog(" {0} {1} {2}", Matches, D->getDeclKindName(), D);
-
- switch (A.Direction) {
- case Anchor::Above:
- if (Matches && !LastMatched) {
- // Special case: if "above" matches an access specifier, we actually
- // want to insert below it!
- if (llvm::isa<AccessSpecDecl>(D)) {
- ReturnNext = true;
- continue;
- }
- return D;
- }
- break;
- case Anchor::Below:
- if (LastMatched && !Matches)
- return D;
- break;
- }
-
- LastMatched = Matches;
- }
- if (ReturnNext || (LastMatched && A.Direction == Anchor::Below))
- return nullptr;
- return llvm::None;
-}
-
SourceLocation beginLoc(const Decl &D) {
auto Loc = D.getBeginLoc();
if (RawComment *Comment = D.getASTContext().getRawCommentForDeclNoCache(&D)) {
@@ -91,6 +47,55 @@
return Spec;
}
+SourceLocation insertionPoint(const DeclContext &DC, Anchor A) {
+ SourceLocation Result;
+ for (auto DIter = DC.decls_begin(); DIter != DC.decls_end(); ++DIter) {
+ const auto *D = *DIter;
+ if (D->isImplicit())
+ continue;
+
+ const Decl *NonTemplate = D;
+ if (auto *TD = llvm::dyn_cast<TemplateDecl>(D))
+ NonTemplate = TD->getTemplatedDecl();
+ bool Matches = A.Match(NonTemplate);
+ dlog(" {0} {1} {2}", Matches, D->getDeclKindName(), D);
+
+ if (Matches) {
+ switch (A.Direction) {
+ case Anchor::Above:
+ // Special case: if "above" matches an access specifier, we actually
+ // want to insert below it!
+ if (!llvm::isa<AccessSpecDecl>(D)) {
+ Result = beginLoc(*D);
+ break;
+ }
+ [[fallthrough]];
+ case Anchor::Below:
+ // We cannot use the `getEndLoc`, because the clang AST does not track,
+ // semicolons. "Below" hence actually means "Before the next one".
+ auto NextIter = DIter;
+ ++NextIter;
+ if (NextIter != DC.decls_end())
+ Result = beginLoc(**NextIter);
+ else if (!DC.isTranslationUnit())
+ // There was no next item. Use the endLoc instead.
+ return endLoc(DC);
+ else {
+ // TranslationUnits don't have a valid endLoc. We need to use the
+ // source manager instead to insert all the way at the bottom of the
+ // file.
+ auto EndLoc = D->getEndLoc();
+ const auto &SM = DC.getParentASTContext().getSourceManager();
+ return SM.getLocForEndOfFile(SM.getFileID(EndLoc));
+ }
+ }
+ if (A.SearchDirection == Anchor::First)
+ break;
+ }
+ }
+ return Result;
+}
+
} // namespace
SourceLocation insertionPoint(const DeclContext &DC,
@@ -98,9 +103,9 @@
dlog("Looking for insertion point in {0}", DC.getDeclKindName());
for (const auto &A : Anchors) {
dlog(" anchor ({0})", A.Direction == Anchor::Above ? "above" : "below");
- if (auto D = insertionDecl(DC, A)) {
- dlog(" anchor matched before {0}", *D);
- return *D ? beginLoc(**D) : endLoc(DC);
+ auto L = insertionPoint(DC, A);
+ if (L.isValid()) {
+ return L;
}
}
dlog("no anchor matched");
@@ -136,7 +141,7 @@
std::vector<Anchor> Anchors,
AccessSpecifier Protection) {
// Fallback: insert at the bottom of the relevant access section.
- Anchors.push_back({any, Anchor::Below});
+ Anchors.push_back({any, Anchor::Below, Anchor::Last});
auto Loc = insertionPoint(InClass, std::move(Anchors), Protection);
std::string CodeBuffer;
auto &SM = InClass.getASTContext().getSourceManager();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
