sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:408
return CB(InpAST.takeError());
- auto &AST = InpAST->AST;
- const auto &SM = AST.getSourceManager();
- auto Loc = sourceLocationInMainFile(SM, Pos);
- if (!Loc)
- return CB(Loc.takeError());
- const auto *TouchingIdentifier =
- spelledIdentifierTouching(*Loc, AST.getTokens());
- if (!TouchingIdentifier)
- return CB(llvm::None); // no rename on non-identifiers.
-
- auto Range = halfOpenToRange(
- SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
- TouchingIdentifier->endLocation()));
-
- if (RenameOpts.AllowCrossFile)
- // FIXME: we now assume cross-file rename always succeeds, revisit this.
- return CB(Range);
-
- // Performing the local rename isn't substantially more expensive than
- // doing an AST-based check, so we just rename and throw away the results.
- auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
- /*GetDirtyBuffer=*/nullptr});
- if (!Changes) {
+ clangd::FileIndex EmptyIndex;
+ // prepareRename is latency-sensitive:
----------------
the empty index is weird here - can we pass nullptr?
Currently nullptr leads to an error in the !crossfile case, but I think we can
give rename(Index=nullptr, RenameOpts.CrossFile=true) the behavior we want.
(otherwise, if we need an empty index use MemIndex())
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:411
+ // - for single-file rename, performing rename isn't substantially more
+ // expensive than doing an AST-based check;
+ // - for cross-file rename, we deliberately pass an empty index to save
the
----------------
mention "the index is used to see if the local rename is complete"?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+ auto Results = clangd::rename(
+ {Pos, "dummy", InpAST->AST, File,
+ RenameOpts.AllowCrossFile ? &EmptyIndex : Index, RenameOpts});
----------------
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)
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:277
+ ///
+ /// The returned result may be incomplete as it only contains occurrences
from
+ /// the current main file.
----------------
nit: drop "may be incomplete as it"?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:500
if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
- return FileEdits(
- {std::make_pair(RInputs.MainFilePath,
- Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
+ return RenameResults{
+ CurrentIdentifier,
----------------
nit: I'd find this more readable as a default construction followed by
assignments to the fields
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:507
- FileEdits Results;
+ FileEdits Edits;
// Renameable safely guards us that at this point we are renaming a local
----------------
and again here - declare the whole struct first and fill it in?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:58
+struct RenameResults {
+ // The range of the symbol that the user can attempt to rename.
----------------
nit: I'd suggest RenameResult singular, consistent with CodeCompleteResult and
ReferencesResult
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:60
+ // The range of the symbol that the user can attempt to rename.
+ Range R;
+ FileEdits Edits;
----------------
Give this a real name... Target?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+ Range R;
+ FileEdits Edits;
+};
----------------
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.
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
