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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68
 
+// Lexes from end of \p FD until it finds a semicolon.
+llvm::Optional<SourceLocation> getSemiColonForDecl(const FunctionDecl *FD) {
----------------
"Lexes" is probably a not very relevant implementation detail.
I'd say this function probably doesn't need a comment, it's clear what it does from its name.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:169
+  tooling::Replacements Replacements;
+  std::string Failure;
+  findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {
----------------
NIT: Maybe store `Error` directly?
It would communicate the intent of the variable more clearly.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:175
+    // qualifier.
+    if (Ref.Qualifier.hasQualifier())
+      return;
----------------
NIT: IIUC, this could be simply `if (Ref.Qualifier)`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:182
+
+    // All Targets should be in the same nested name scope, so we can safely
+    // chose any one of them.
----------------
As discussed before, `Targets` can actually come from different scopes, e.g. from ADL.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+    // All Targets should be in the same nested name scope, so we can safely
+    // chose any one of them.
+    const NamedDecl *ND = Ref.Targets.front();
----------------
It just occurred to me that this is a really bad idea. I **think** the order of items in `Targets` is non-deterministic.
Could we instead try to check if all scopes are the same and only qualify in that case?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196
+
+    std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+    if (auto Err = Replacements.add(
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:199
+            tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
+      Failure = llvm::formatv("Failed to add qualifier: {0}",
+                              llvm::fmt_consume(std::move(Err)));
----------------
Maybe return a generic error in the end instead of the last error?
Something like `createStringError(..., "Failed to compute qualifiers, see errors in the logs for more details")`.

Should be a better UX for anyone who tries to explore what went wrong.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:223
+
+/// Generates Replacements for changing parameter names in \p Dest to be the
+/// same as in \p Source.
----------------
NIT: mention that both function parameters and template parameters are updated here.


================
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();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647