hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+ auto Results = clangd::rename(
+ {Pos, "dummy", InpAST->AST, File,
+ RenameOpts.AllowCrossFile ? &EmptyIndex : Index, RenameOpts});
----------------
sammccall wrote:
> we're now returning the "dummy" string to the caller, so we should document
> it somewhere (or ideally just make it the empty string and document that)
make it empty string, and added document.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+ Range R;
+ FileEdits Edits;
+};
----------------
sammccall wrote:
> It's slightly awkward to have the half-populated state (may or may not
> contain cross-file results, can't tell without the query).
>
> I'd consider redundantly including `Edit LocalChanges` and `FileEdits
> GlobalChanges` with the former always set and the latter empty when returned
> from preparerename.
> It's slightly awkward to have the half-populated state (may or may not
> contain cross-file results, can't tell without the query).
I feel this is not too bad, the query is quite trivial, just `Edits.size() > 1`.
> I'd consider redundantly including Edit LocalChanges and FileEdits
> GlobalChanges with the former always set and the latter empty when returned
> from preparerename.
This change seems nice to get changes for main-file, but I think
`GlobalChanges` should include LocalChanges (otherwise, client side needs to
combine these two when applying the final rename edits)? then we can't leave
the later empty while keeping the former set.
Happy to do the change, but it looks like we don't have usage of `LocalChanges`
so far. In prepareRename, we want main-file occurrences, `rename` will always
return them regardless of single-file or cross-file rename, so I think `Edits`
is efficient.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88634/new/
https://reviews.llvm.org/D88634
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
