[PATCH] D75429: [clangd] DefineOutline removes `override` specified from overridden methods.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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