[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

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