kadircet added a comment.
Thanks for working on this!
A few comments on macro handling and coding style. Apart from that mostly needs
more testing.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:216
+ // Remove the virtual, override and final specifiers.
+ if (FD->hasAttrs()) {
+ for (auto *Attr : FD->getAttrs()) {
----------------
nit:
```
auto DelAttr = [&](const Attr* A) { /* do magic */};
if(auto *OA = FD->getAttr<OverrideAttr>())
DelAttr(OA);
if(auto *FA = ...)
```
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:219
+ if (isa<OverrideAttr>(Attr) || isa<FinalAttr>(Attr)) {
+ assert(Attr->getLocation().isValid());
+ if (Attr->getLocation().isMacroID()) {
----------------
can you rather use `auto AttrToken =
TB.getSpelledTokens(TB.getExpandedTokens(Attr.getRange()));` throughout this
part.
It can provide you with token range as well.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:220
+ assert(Attr->getLocation().isValid());
+ if (Attr->getLocation().isMacroID()) {
+ Errors = llvm::joinErrors(
----------------
can you add some test cases for this branch ? In theory it should be ok to drop
this if full expansion is just the attr, i.e.
```
#define FNL final
struct A { virtual void foo() FNL {} };
```
but should possibly fail in :
```
#define MACRO foo() final
struct A { virtual void MACRO {} };
```
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:236
+ if (auto Err =
+ QualifierInsertions.add(tooling::Replacement(SM, DelRange,
"")))
+ Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
----------------
nit: I believe `QualifierInsertions` needs to be renamed now, maybe something
like `DeclarationCleanups` ?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:244
+ bool Any = false;
+ // Clang allows duplicating virtual specifiers so check for multiple
+ // occurances.
----------------
again could you please add tests checking this case?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:246
+ // occurances.
+ for (const syntax::Token &Tok : TokBuf.expandedTokens(SpecRange)) {
+ if (Tok.kind() == tok::kw_virtual) {
----------------
you would rather want to go over spelled tokens, as expandedtokens might not
exist in the source code. (it looks like the usage above, not related to this
patch, is also broken. No need to fix that one I'll try to prepare a fix, but
patches welcome.)
`TokBuf.spelledForExpanded(TokBuf.expandedTokens(SpecRange))`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:247
+ for (const syntax::Token &Tok : TokBuf.expandedTokens(SpecRange)) {
+ if (Tok.kind() == tok::kw_virtual) {
+ assert(Tok.location().isValid());
----------------
nit: use early exits, i.e:
```
if(Tok.kind() != tok::kw_virtual)
continue;
``
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:249
+ assert(Tok.location().isValid());
+ if (Tok.location().isMacroID()) {
+ Errors =
----------------
same argument as above.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:260
+ CharSourceRange DelRange =
+ CharSourceRange::getTokenRange(Tok.location());
+ if (auto Err =
----------------
you can use `Tok.range(SM)` instead
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2113
+ struct B : A {
+ void foo() final ;
+ };)cpp",
----------------
can you also add a test case for `final override`/`override final`
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
