[PATCH] D77534: [clangd] DefineOutline: removes static token from static CXXMethodDecl

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:242
 
-  if (FD->isVirtualAsWritten()) {
-    SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
-    bool HasErrors = true;
-
-    // Clang allows duplicating virtual specifiers so check for multiple
-    // occurances.
-    for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
-      if (Tok.kind() != tok::kw_virtual)
+  auto DeleteKeyword = [&](tok::TokenKind Kind, SourceRange FromRange,
+                           bool AllowMultiple = false) {
----------------
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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:242
 
-  if (FD->isVirtualAsWritten()) {
-    SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
-    bool HasErrors = true;
-
-    // Clang allows duplicating virtual specifiers so check for multiple
-    // occurances.
-    for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
-      if (Tok.kind() != tok::kw_virtual)
+  auto DeleteKeyword = [&](tok::TokenKind Kind, SourceRange FromRange,
+                           bool AllowMultiple = false) {
----------------
kadircet wrote:
> 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.
nit: s/DeleteKeyword/DelKeyword/

to be consistent with `DelAttr`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:248
         continue;
+      if (FoundAny && !AllowMultiple) {
+        Errors = llvm::joinErrors(
----------------
this condition becomes obsolete once you get rid of AllowMultiple.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:267
+                (StringRef(
+                     "define outline: can't move outline as can't remove `") +
+                 tok::getKeywordSpelling(Kind) + "` keyword.")
----------------
```
llvm::formatv("define outline: couldn't remove `{0}` keyword.", tok::getKeywordSpelling(Kind))
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:279
     }
-    if (HasErrors) {
+    if (!FoundAny)
       Errors = llvm::joinErrors(
----------------
nit: put braces here as statement spans multiple lines


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:285
+              (StringRef(
+                   "define outline: can't move outline as couldn't find `") +
+               tok::getKeywordSpelling(Kind) + "` keyword to remove.")
----------------
please use formatv here as well, while dropping `can't move outline`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:290
+
+  if (FD->isVirtualAsWritten())
+    DeleteKeyword(tok::kw_virtual, {FD->getBeginLoc(), FD->getLocation()},
----------------
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);
}


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:294
+  if (isa<CXXMethodDecl>(FD) && cast<CXXMethodDecl>(FD)->isStatic())
+    DeleteKeyword(tok::kw_static, {FD->getBeginLoc(), FD->getLocation()});
 
----------------
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`


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2152
+            struct A {
+              static void foo() ;
+            };)cpp",
----------------
please also add a test with multiple static keywords


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77534/new/

https://reviews.llvm.org/D77534