[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

We also need to rename parameters sometimes, right?

  // Sometimes we need to rename parameters.
  void usages(int decl_param, int);
  
  void usages(int def_param, int now_named) {
    llvm::errs() << def_param + now_named;
  }
  
  // And template parameters! (these are even more interesting)
  template <class T>
  struct Foo {
    template <class U, class>
    void usages();
  };
  template <class L>
  template <class R, class NowNamed>
  void Foo<L>::usages() {
    llvm::errs() << L() + R() + NowNamed();
  }



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:170
+
+    // All Targets should be in the same nested name scope, so we can safely
+    // chose any one of them.
----------------
I don't think it's true in presence of using declarations:
```
namespace ns {
  void foo(int*);
}
namespace other_ns {
  void foo(char*);
}

using ns::foo;
using other_ns::foo;

template <class T>
void usages() {
  foo(T()); // <-- would we add `ns::foo` or `other_ns::foo`?
}
```

Not qualifying in this case is probably ok, but adding any of the qualifiers is probably wrong.
Could you check what we do in this case and either add to tests or put into a list of known issues (e.g. file a bug or at least add a FIXME?)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:176
+      return;
+    // Skip members as qualyfying the left side is enough.
+    if (ND->isCXXInstanceMember())
----------------
NIT: could you add an example of a member expression here?

Note that sometimes there is no left side (it's implicit), but in that case we shouldn't qualify either.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:177
+    // Skip members as qualyfying the left side is enough.
+    if (ND->isCXXInstanceMember())
+      return;
----------------
I believe we could also avoid qualifying anything from non-namespace scope.
This follows from the fact that one cannot move stuff from class to namespace with using declarations:
```
namespace ns {
class X {
  static void foo();
  void usages();
};
}

using ns::X::foo; // <- not allowed!
void ns::X::usages() {
  foo(); // <-- so no need to qualify this!
}
```
If I'm reading the code correctly, we will get `ns::foo()` in the current version.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:227
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+    return FTD->getBeginLoc();
----------------
Not sure if this covers these two cases (could you please add them to the tests?):
```
template <class T>
struct vector {
  void foo();
};

template <class T>
void vector<T>::foo() {}

template <class T>
struct list {
  template <class U>
  void foo();
}

template <class T>
template <class U>
void list<T>::foo() {}
```

Or are we disabled in those cases?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:301
+    tooling::Replacements Replacements;
+    auto Err = Replacements.add(
+        tooling::Replacement(SM, *SemiColon, 1, *QualifiedBody));
----------------
This never fails since there are no replacements to conflict with, right?
Maybe we could store a single `tooling::Replacement` here instead? That should simplify rest of the code (e.g. no need to `clear()` replacements and re-use them for different file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647