[PATCH] D70594: [clangd] Implement range patching heuristics for cross-file rename.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 5 10:52:38 PST 2019
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:384
+bool isLineOrColumnEqual(const Position &LHS, const Position &RHS) {
+ return LHS.line == RHS.line || LHS.character == RHS.character;
----------------
the name here isn't much simpler/clearer than the code.
`impliesSimpleEdit`?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:442
+private:
+ void enumerate(size_t NextMatched, size_t NextLexed, size_t IterCount) {
+ if (IterCount > KMaxIterations) {
----------------
it looks like itercount at the moment is counting depth and ignoring breadth.
I think you want to pass by reference and increment on each call, instead.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:442
+private:
+ void enumerate(size_t NextMatched, size_t NextLexed, size_t IterCount) {
+ if (IterCount > KMaxIterations) {
----------------
sammccall wrote:
> it looks like itercount at the moment is counting depth and ignoring breadth.
>
> I think you want to pass by reference and increment on each call, instead.
FWIW, this seems like a class that could be a function:
params are:
```
vector<int> &PartialMatch
ArrayRef<Range> IndexedRest // just those not covered by PartialMatch
ArrayRef<Range> MatchedRest // those still to consider
int &Fuel // set to 10000, decrement, bail out once it goes negative
Callback // as now, though no need for the parameter
```
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:442
+private:
+ void enumerate(size_t NextMatched, size_t NextLexed, size_t IterCount) {
+ if (IterCount > KMaxIterations) {
----------------
sammccall wrote:
> sammccall wrote:
> > it looks like itercount at the moment is counting depth and ignoring breadth.
> >
> > I think you want to pass by reference and increment on each call, instead.
> FWIW, this seems like a class that could be a function:
> params are:
>
> ```
> vector<int> &PartialMatch
> ArrayRef<Range> IndexedRest // just those not covered by PartialMatch
> ArrayRef<Range> MatchedRest // those still to consider
> int &Fuel // set to 10000, decrement, bail out once it goes negative
> Callback // as now, though no need for the parameter
> ```
you can bail out as soon as there are not enough lexed tokens remaining to match
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:452
+ for (int Pos : MatchedIndex)
+ Mapped.push_back(Lexed[Pos]);
+ return MatchedCB(std::move(Mapped));
----------------
if we're actually evaluating all ranges, can we pass the index array (by reference), use it to evaluate scores, and only copy ranges for the winner?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:456
+
+ for (size_t I = NextLexed; I < Lexed.size(); ++I) {
+ if (Visited.count(I))
----------------
This works but maybe more common/obvious is to use recursion for both cases instead of the loop:
```
if (isLineOrColumnEqual(..., Lexed[NextLexed])){
// match
...
enumerate(NextMatched+1, NextLexed+1);
}
// don't match
enumerate(NextMatched, NextLexed+1);
```
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:457
+ for (size_t I = NextLexed; I < Lexed.size(); ++I) {
+ if (Visited.count(I))
+ continue;
----------------
Visited seems redundant: it always contains [0, NextLexed)
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:673
+ std::vector<Diff> Diffs;
+ for (size_t I = 0; I < Indexed.size(); ++I)
+ Diffs.push_back(computeDiff(Indexed[I], Mapped[I]));
----------------
Not clear to me why we're building these structures.
The cost is a sum of implied edits, implied edits are a function of (last displacement, current displacement, are they on the same line)
so what about something like:
```
Cost = 0;
LastLine = -1;
LastDX = 0, LastDY = 0;
for (I) {
DX = Mapped[I].begin.character - Indexed[I].begin.character;
DY = Mapped[I].begin.line - Indexed[I].begin.line;
Line = Mapped[I].begin.line;
if (!(Line == LastLine && DX == LastDX))
LastDX = 0; // horizontal offsets don't carry across lines
Cost += abs(DX - LastDX) + abs(DY - LastDY);
LastDX, LastDY, LastLine = DX, DY, Line;
}
```
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:59
+/// Gets "authoritative" occurrences to perform the rename.
+///
----------------
I'm not sure if authoritative is accurate here - authoritative would be from an AST or fresh index.
I'd suggest "Adjusts indexed occurrences to match the current state of the file"
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+///
+/// Index is not always up-to-update. Applying stale indexed occurrences leads
+/// to bad rename result, so this API helps to mitigate the stale-index issue.
----------------
nit: The index is not always up to date.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:62
+/// Index is not always up-to-update. Applying stale indexed occurrences leads
+/// to bad rename result, so this API helps to mitigate the stale-index issue.
+/// It determines the indexed occurrences are good enough, and heuristically
----------------
"bad rename result" is vague.
Maybe "Blindly editing at the locations reported by the index may mangle the code in such cases".
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:62
+/// Index is not always up-to-update. Applying stale indexed occurrences leads
+/// to bad rename result, so this API helps to mitigate the stale-index issue.
+/// It determines the indexed occurrences are good enough, and heuristically
----------------
sammccall wrote:
> "bad rename result" is vague.
> Maybe "Blindly editing at the locations reported by the index may mangle the code in such cases".
"This API helps" doesn't add much here - maybe merge with the next sentence?
"This function determines whether the indexed occurrences can be applied to this file, and heuristically..."
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:66
+///
+/// Details:
+/// - lex the draft code to get all rename candidates, this yields a superset
----------------
The details comment belongs in the implementation rather than the header, I think.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:75
+/// positives (e.g. candidates got appended), but rename is still safe
+/// (b) index returns non-candidate results, we use the best near miss.
+///
----------------
"we use the best near miss" - it would help to describe approximately what this means. It's very hard to understand that this can fail because no candidate is "near".
e.g. "we attempt to map the indexed locations onto candidates in a plausible way (e.g. guess that lines were inserted). If such a "near miss" is found, the rename is still possible"
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:77
+///
+/// The API assumes that IndexedOccurrences contains only named occcurrences (
+/// each occurrence has the same length).
----------------
nit: occcurrences -> occurences
nit: break before (
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:79
+/// each occurrence has the same length).
+llvm::Expected<std::vector<Range>> patchRanges(llvm::StringRef DraftCode,
+ llvm::StringRef Identifier,
----------------
I think this use of "patch" is a little confusing, and the name is a bit vague for the clangd namespace.
`adjustRenameRanges`?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:84
+
+/// Evaluates how good the mapping result is. 0 indicates a perfect match.
+///
----------------
"the mapping result" doesn't make sense in this context (the header), as you haven't exposed/described the function that computes mappings or otherwise defined the concept.
(I think exposing the function would be best both for clarity and for unit-tests)
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:86
+///
+/// It uses the sum of the implied edit sizes between successive diffs, only
+/// simple edits are considered:
----------------
I'd suggest moving most of this comment (at least the examples) to the cpp file.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:103
+/// REQUIRED: Indexed and Mapped are sorted, and have the same size.
+size_t editCost(ArrayRef<Range> Indexed, ArrayRef<Range> Mapped);
+
----------------
`editCost` is again not a specific enough name for this operation.
The cost we're describing is specific to *adjusting* the indexed ranges to correspond to the mapped ones. Maybe `renameRangeAdjustmentCost()`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70594/new/
https://reviews.llvm.org/D70594
More information about the cfe-commits
mailing list