[clangd] DefineOutline: removes static token from static CXXMethodDecl

[njames93]

Summary

Removes the static token when defining a function out of line if the function is a CXXMethodDecl

[kadircet]

thanks for the patch!

this is quite similar to handling kw_virtual even handling multiple static keywords. Could you rather factor the search for kw, generate an edit to delete specifier part into a DelKeyword lambda similar to DelAttr and call it with kw_static and kw_virtual?

p.s. you can make use of tok::getKeywordSpelling for error messages.

[njames93]

• Moved keyword removing logic into a lambda.

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

242–245

nit, AllowMultiple is always false(see my comment below) maybe just drop that parameter and add a comment to lambda saying that it deletes all occurences of the keyword in range.

242–245

nit: s/DeleteKeyword/DelKeyword/

to be consistent with DelAttr

247

this condition becomes obsolete once you get rid of AllowMultiple.

255

llvm::formatv("define outline: couldn't remove `{0}` keyword.", tok::getKeywordSpelling(Kind))

265

nit: put braces here as statement spans multiple lines

284

please use formatv here as well, while dropping can't move outline

289

nit: this also needs to be a methoddecl, so you can combine the two checks:

if(const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {

if(MD->isvirt...) delKeyword(virt);
if(MD->isStatic) delKeyword(static);

}

293

sorry if I miscommunicated, I was trying to say that multiple static keywords are also allowed by clang. So we should be dropping all of them, as we do for virtual

clang-tools-extra/clangd/unittests/TweakTests.cpp

2152

please also add a test with multiple static keywords

[njames93]

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

293

Yeah sorry, I checked and it was rejected. I just realised I was checking against gcc...

[njames93]

• Address comments

[njames93]

• Fix issue with last revision

[kadircet]

thanks LGTM!

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

auto DelAttr = [&](const Attr *A) {
     if (auto Err =
             DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
       Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
   };
 
   DelAttr(FD->getAttr<OverrideAttr>());
   DelAttr(FD->getAttr<FinalAttr>());
 
-  if (FD->isVirtualAsWritten()) {
-    SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
-    bool HasErrors = true;
-
-    // Clang allows duplicating virtual specifiers so check for multiple
-    // occurrences.
-    for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
-      if (Tok.kind() != tok::kw_virtual)
+  auto DelKeyword = [&](tok::TokenKind Kind, SourceRange FromRange) {
+    bool FoundAny = false;
+    for (const auto &Tok : TokBuf.expandedTokens(FromRange)) {
+      if (Tok.kind() != Kind)
         continue;
+      FoundAny = true;
       auto Spelling = TokBuf.spelledForExpanded(llvm::makeArrayRef(Tok));
       if (!Spelling) {
-        HasErrors = true;
+        Errors = llvm::joinErrors(
+            std::move(Errors),
+            llvm::createStringError(
+                llvm::inconvertibleErrorCode(),
+                llvm::formatv("define outline: couldn't remove `{0}` keyword.",
+                              tok::getKeywordSpelling(Kind))));
         break;
       }
-      HasErrors = false;
       CharSourceRange DelRange =
           syntax::Token::range(SM, Spelling->front(), Spelling->back())
               .toCharRange(SM);
       if (auto Err =
               DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
         Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
     }
-    if (HasErrors) {
+    if (!FoundAny) {
       Errors = llvm::joinErrors(
           std::move(Errors),
-          llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                  "define outline: Can't move out of line as "
-                                  "function has a macro `virtual` specifier."));
+          llvm::createStringError(
+              llvm::inconvertibleErrorCode(),
+              llvm::formatv(
+                  "define outline: couldn't find `{0}` keyword to remove.",
+                  tok::getKeywordSpelling(Kind))));
     }
+  };
+
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
+    if (MD->isVirtualAsWritten())
+      DelKeyword(tok::kw_virtual, {FD->getBeginLoc(), FD->getLocation()});
+    if (MD->isStatic())
+      DelKeyword(tok::kw_static, {FD->getBeginLoc(), FD->getLocation()});
   }
 
   if (Errors)
     return std::move(Errors);
   return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
 }
 
 struct InsertionPoint {
   std::string EnclosingNamespace;
   size_t Offset;
 };
 // Returns the most natural insertion point for \p QualifiedName in \p Contents.
 // This currently cares about only the namespace proximity, but in feature it
 // should also try to follow ordering of declarations. For example, if decls
 // come in order `foo, bar, baz` then this function should return some point
 // between foo and baz for inserting bar.
 llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
                                                  llvm::StringRef QualifiedName,
                                                  const LangOptions &LangOpts) {
   auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
 

clang-tools-extra/clangd/unittests/TweakTests.cpp

llvm::StringRef ExpectedSource;
             struct A {
               virtual void foo() = 0;
             };
             struct B : A {
               void foo() final override ;
             };)cpp",
           "void B::foo()   {}\n",
       },
+      {
+          R"cpp(
+            struct A {
+              static void fo^o() {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              static void foo() ;
+            };)cpp",
+          " void A::foo() {}\n",
+      },
+      {
+          R"cpp(
+            struct A {
+              static static void fo^o() {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              static static void foo() ;
+            };)cpp",
+          "  void A::foo() {}\n",
+      },
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
llvm::StringRef ExpectedSource;
           struct A {
             STUPID_MACRO(sizeof sizeof int) void f^oo() {}
           };)cpp",
        R"cpp(#define STUPID_MACRO(X) virtual
           struct A {
             STUPID_MACRO(sizeof sizeof int) void foo() ;
           };)cpp",
        " void A::foo() {}\n"},
+      {R"cpp(#define STAT static
+          struct A {
+            STAT void f^oo() {}
+          };)cpp",
+       R"cpp(#define STAT static
+          struct A {
+            STAT void foo() ;
+          };)cpp",
+       " void A::foo() {}\n"},
+      {R"cpp(#define STUPID_MACRO(X) static
+          struct A {
+            STUPID_MACRO(sizeof sizeof int) void f^oo() {}
+          };)cpp",
+       R"cpp(#define STUPID_MACRO(X) static
+          struct A {
+            STUPID_MACRO(sizeof sizeof int) void foo() ;
+          };)cpp",
+       " void A::foo() {}\n"},
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
 
   std::pair<StringRef, StringRef> Cases[] = {
       {
           R"cpp(
           #define VIRT virtual void
           struct A {
             VIRT fo^o() {}
           };)cpp",
-          "fail: define outline: Can't move out of line as function has a "
-          "macro `virtual` specifier."},
+          "fail: define outline: couldn't remove `virtual` keyword."},
       {
           R"cpp(
           #define OVERFINAL final override
           struct A {
             virtual void foo() {}
           };
           struct B : A {
             void fo^o() OVERFINAL {}