[PATCH] D70594: [clangd] Implement range patching heuristics for cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:452
+      for (int Pos : MatchedIndex)
+        Mapped.push_back(Lexed[Pos]);
+      return MatchedCB(std::move(Mapped));
----------------
sammccall wrote:
> 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?
we could use the index array to evaluate the scores, but it would make the cost API signature a bit weird, like `size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed, ArrayRef<size_t> MatchedLexedIndex);`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:95
+  /// \p getBest, exposing for testing only.
+  static MatchType match(llvm::ArrayRef<Range> LHS, llvm::ArrayRef<Range> RHS);
+
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > Oops, forgot this...
> > > I think the public API isn't quite right here - exposing parts for testing is fine, but the classification itself isn't fine grained enough I think. (Too easy to write a test that "passes" but the actual mapping found isn't the right one).
> > > 
> > > And the class structure wrapping a LangOpts ref seems like a detail that can be hidden.
> > > 
> > > I'd like to see:
> > >  - a function that returns the lexed ranges from a StringRef/LangOpts
> > >  - a function that constructs the mapping given two sequences of ranges (like `getMappedRanges(ArrayRef<Range>, ArrayRef<Range>) -> vector<Range>`
> > >  - a function that ties these together to the data structures we care about (i.e. taking Code + identifier + LangOpts + ArrayRef<Ref> or so)
> > > 
> > > then you can unit test the first and second and smoke test the third.
> > > 
> > > Tests like
> > > 
> > > ```
> > > Indexed = "int [[x]] = 0; void foo(int x);";
> > > Draft = "double [[x]] = 0; void foo(double x);";
> > > verifyRenameMatches(Indexed, Draft);
> > > ```
> > > a function that returns the lexed ranges from a StringRef/LangOpts
> > 
> > There is an existing function `collectIdentifierRanges` in SourceCode.cpp, and it has been unittested.
> > 
> > 
> > > a function that constructs the mapping given two sequences of ranges (like getMappedRanges(ArrayRef<Range>, ArrayRef<Range>) -> vector<Range>
> > > a function that ties these together to the data structures we care about (i.e. taking Code + identifier + LangOpts + ArrayRef<Ref> or so)
> > 
> > sure, I think it is sufficient to test the second one, since the second one is a simple wrapper of the `getMappedRanges`.
> > sure, I think it is sufficient to test the second one, since the second one is a simple wrapper of the `getMappedRanges`.
> 
> Did you mean "sufficient to test the first one"?
> Testing the second one is certainly sufficient, but tests more than it needs to (particularly the lexing bits again).
I got the point here, exposed `getMappedRanges` and added unit tests for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70594/new/

https://reviews.llvm.org/D70594