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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196
+
+    std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+    if (auto Err = Replacements.add(
----------------
ilya-biryukov wrote:
> You would want to use `ND->printNestedNameSpecifier()` instead to avoid printing inline namespaces:
> ```
> namespace std { inline namespace v1 { 
>  struct vector {};
> }}
> ```
> 
> ^-- I believe the current code would print `std::v1::` for `vector` and we want `std::`.
> Could you add this example to tests too?
it is the opposite, `printNamespaceScope` skips anonymous and inline namespaces, whereas `printNestedNameSpecifier` also prints those. adding a test.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226
+llvm::Expected<tooling::Replacements>
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  const SourceManager &SM = Dest->getASTContext().getSourceManager();
----------------
ilya-biryukov wrote:
> This does not rename any references to those parameters, right?
> E.g.
> ```
> template <class T> void foo(T x);
> 
> template <class U> void foo(U x) {}
> ```
> 
> would turn into
> ```
> template <class U> void foo(T x);
> ```
> right?
yes that's right, just adding a unittest will address this later on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647