[PATCH] D75429: [clangd] DefineOutline won't copy virtual specifiers on methods

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM with a few small comments, please address those before landing.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:262
+      }
+      assert(Spelling->size() == 1);
+      if (auto Err = DeclarationCleanups.add(tooling::Replacement(
----------------
kadircet wrote:
> why assert on this and not delete the whole Spelling that produced the virtual keyword ?
> 
> e.g.
> 
> 
> ```
> #define STUPID_MACRO(X) virtual
> struct F {
> STUPID_MACRO(BLA BLA) void foo();
> }
> ```
> 
> 
sorry for not explicitly asking in the previous one, could you also add a test for this case?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:244
+    SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
+    bool HasNoErrors = false;
+
----------------
double negation is bad(this already looks confusing, sounds like we start in an errorful state?) I think we should rather have `HadErrors`


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2205
+          };)cpp",
+        " void A::foo() {}\n",},
+      {R"cpp(
----------------
you also need to delete the comma just before the `}`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75429