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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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);
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70594