sammccall added a comment. High level comments based on offline discussion:
I think we want to define/formalize the concept of a near miss, to make precise the tradeoffs between false positives, false negatives, and implementability. Not just at the individual level (an index occurrence vs a lexed occurrence) but at a whole-document level. A possible definition, intuitively finding the locations where the indexed occurrences are now spelled: - a near miss maps all of the **name** occurrences in the index onto a **subset** of the lexed occurrences. (names may refer to more than one thing) - indexed occurrences must all be mapped. Result must be distinct, and preserve order. (Support only simple edits to ensure our mapping is robust) - each indexed->lexed correspondence may change row or column but not both (increases chance our mapping is robust) Then we can use a greedy algorithm to find a match (or memoized DFS to enumerate/compare all matches) A good metric for comparing competing mappings might be the sum of the implied edit sizes between successive entries. i.e. if the first three mappings are offset by 1 column down, and the last is offset by 2 columns down, then this implies an insertion of 1 line at the top of the file and 1 line between edits 3 and 4, for a total of 2. subset is fine/good to handle as a special case - if the subset is small we don't want to find it by search. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:385 + assert(std::is_sorted(Superset.begin(), Superset.end())); + auto ItSubset = Subset.begin(); + auto ItSuperset = Superset.begin(); ---------------- I think this is just return std::includes(Superset.begin, Superset.end(), Subset.begin(), Subset.end()); (probably clear enough to just inline to the callsite) ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:623 + if (M == MatchType::Subset) + return std::move(IndexOccurrences); // RVO is disallowed for parameters. + if (M == MatchType::NearMiss) ---------------- That doesn't mean you need std::move to get a move, it just means you can't avoid calling the move constructor. https://godbolt.org/z/Rh-CvT 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
