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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
