ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:326
+ // class Foo, but the token under the cursor is not corresponding to the
+ // "Foo" range, though the final result is correct.
SourceLocation Loc = getBeginningOfIdentifier(
----------------
hokein wrote:
> ilya-biryukov wrote:
> > I would argue rename should not work in that case.
> > Could we check that the cursor is actually on the name token of the
> > `NamedDecl` and not rename if it isn't?
> you are probably right, we only allow rename which is triggered on the name
> token. Will update the patch.
Definitely think we should do it before landing the patch.
Otherwise we'll introduce regressions.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+ tooling::getCanonicalSymbolDeclaration(&RenameDecl),
AST.getASTContext());
+ llvm::DenseSet<SymbolID> TargetIDs;
+ for (auto &USR : RenameUSRs)
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Why `USRs`? Could we just check whether the `ND.getCanonicalDecl()` is
> > there instead?
> checking `ND.getCanonicalDecl()` is not enough, thinking about virtual
> methods.
>
> `tooling::getUSRsForDeclaration` does all the black-magic things here, it
> returns all rename-related decls.
Could you add a comment that we need this to handle virtual methods?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:185
+ for (const auto *Target : Ref.Targets) {
+ auto ID = getSymbolID(Target);
+ assert(ID);
----------------
Are we sure that USRs will always work here?
I would be protective here, surely there are unhandled cases.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:225
tooling::Replacements FilteredChanges;
- for (const tooling::SymbolOccurrence &Rename :
- findOccurrencesWithinFile(AST, RenameDecl)) {
- // Currently, we only support normal rename (one range) for C/C++.
- // FIXME: support multiple-range rename for objective-c methods.
- if (Rename.getNameRanges().size() > 1)
- continue;
- // We shouldn't have conflicting replacements. If there are conflicts, it
- // means that we have bugs either in clangd or in Rename library, therefore
- // we refuse to perform the rename.
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ // We shouldn't have conflicting replacements or replacements from
different
----------------
This seems to assume all occurrences are outside macros.
Won't it break in presence of macros?
Do we have tests when the renamed token is:
- inside macro body
- inside macro arguments
?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69934/new/
https://reviews.llvm.org/D69934
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
