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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
