diff --git a/clang-tools-extra/clangd/refactor/InsertionPoint.h b/clang-tools-extra/clangd/refactor/InsertionPoint.h --- a/clang-tools-extra/clangd/refactor/InsertionPoint.h +++ b/clang-tools-extra/clangd/refactor/InsertionPoint.h @@ -23,6 +23,8 @@ std::function 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. diff --git a/clang-tools-extra/clangd/refactor/InsertionPoint.cpp b/clang-tools-extra/clangd/refactor/InsertionPoint.cpp --- a/clang-tools-extra/clangd/refactor/InsertionPoint.cpp +++ b/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 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(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(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(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(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 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(); diff --git a/clang-tools-extra/clangd/unittests/InsertionPointTests.cpp b/clang-tools-extra/clangd/unittests/InsertionPointTests.cpp --- a/clang-tools-extra/clangd/unittests/InsertionPointTests.cpp +++ b/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(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 { + return [S](const Decl *D) { + if (const auto *ND = llvm::dyn_cast(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 void privTemplateMethod(); + $template^template 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{});