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); + ---------------- 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); ``` 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
