sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
LG thanks! ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:496 + auto CurrentStartIt = CurrentContentsToLine.find(OldLines[OldStart]); + if (CurrentStartIt == CurrentContentsToLine.end()) + return false; ---------------- up to you, but I find avoiding iterators easier to read `for (int AlternateLine : CurrentContentsToLine.lookup(OldLines[OldStart]))` (or with extracted variables) ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:509 + // Check if AlternateLine matches all lines in the range. + if (llvm::any_of( + llvm::zip_equal(RangeContents, ---------------- why not just `if (RangeContents != CurrentLines.slice(...))`? I feel like I'm missing something subtle :-) Also if you replace the `slice(a, b)` with `drop_front(a).take_front(b)` then you don't need the `CurrentEnd > CurrentLines.size()` check ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:517 + + if (Closest == -1 || + abs(AlternateLine - OldStart) < abs(Closest - OldStart)) ---------------- if you make `Closest` the delta rather than the line number this gets a bit easier to follow IMO: `abs(Closest) < abs(Delta)` here `R.start.line += Closest; R.end.line += Closest;` below You need to make `Closest` an `optional<int>` but that's also clearer than a sentinel -1 for me ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:568 + + Diag NewD = D; + NewD.Range = NewRange; ---------------- FWIW if we're willing to pay a copy for a successful patch, I don't think it's worth any hassle to avoid the copy for a failed patch. Up to you though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/ https://reviews.llvm.org/D143096 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
