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