[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D68937#1710915 <https://reviews.llvm.org/D68937#1710915>, @kadircet wrote:

> I totally agree that the solution you proposed would also work, but don't think it would be any less code. Since one needs to correlate
>  parameters between two different declarations, and `findExplicitReferences` doesn't really provide a nice way to achieve that. One
>  would have to rely on `SourceLocation` ordering or in the order callback was issued(which implicitly relies on AST traversal order).
>
> It would be nice to unify the rewrite parameter name/type/defaultarg logics, but not sure if it is worth.


I believe it should be less and much simpler code. In particular, I propose something like this:

  DenseSet<const NameDecl*> ParametersToRename;
  // Fill ParametersToRename with template and function parameters.
  
  DenseMap<const NamedDecl*, std::vector<SourceLocation>> References;
  findExplicitReferences(Func, [&](ReferenceLoc R) {
    if (!ParametersToRename.count(R.Target))
      return;
    References[R.Target].push_back(R.NameLoc);
  });
  
  for (auto [Param, Locations] : References) {
    auto NewName = NewNameForParam(Param);
    for (auto L : Locations)
      Replacements.add(replaceToken(L, NewName));
  }

That should also handle various interesting cases like `try-catch` blocks and constructor initializer lists.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:229
 
+/// Generates a replacement that renames \p Dest to \p Source.
+llvm::Error rewriteParameterName(tooling::Replacements &Replacements,
----------------
NIT: `that renames \p Dest to \p SourceName`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:232
+                                 const NamedDecl *Dest,
+                                 llvm::StringRef SourceName) {
+  auto &SM = Dest->getASTContext().getSourceManager();
----------------
NIT: call it `NewName`?
`Source` and `Target` are very define-inline-specific. This function actually does a more general thing and using the define-inline terminology hurt the redability a little.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:239
+          SM, Dest->getLocation(), DestName.size(),
+          // If \p Dest is unnamed, we need to insert a space before current
+          // name.
----------------
Why do we need to insert a space here? could you add an example?
I guess it's 
```
void foo(int^) // <-- avoid gluing 'int' and the new name here
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:241
+          // name.
+          ((DestName.empty() ? " " : "") + SourceName).str()))) {
+    return Err;
----------------
NIT: could you move this expression and a comment to a separate variable?
should simplify the if condition and improve readability


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68937