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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
