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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:625
+// same spelling.
+static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
+                                    const ScannedPreamble &BaselineScan,
----------------
I think this function is too long with too many local lambdas, consider converting it to a class instead (call me old-fashioned!)


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+    auto ModifiedStartIt =
+        ModifiedContentsToLine.find(BaselineScan.Lines[BaselineStart]);
+    if (ModifiedStartIt == ModifiedContentsToLine.end())
----------------
we're assuming that the ranges are within the preamble and everyone agrees about the bounds. If not, BaselineStart may not be a valid index into BaselineScan.Lines

This assumption seems broadly reasonable, but is *very much* not locally enforced. I'd suggest a defensive check or at least an assert.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:648
+      return std::nullopt;
+    int Closest = ModifiedStartIt->second.front();
+    for (auto AlternateLine : ModifiedStartIt->second) {
----------------
this doesn't look right: you're first deciding which possible starting point is closest, and then deciding whether it matches. So a range that matches can be masked by a range where only the first line matches, if the latter is closer.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:683
+      NewFix.Edits.emplace_back(E);
+      NewFix.Edits.back().range = *NewRange;
+    }
----------------
probably doesn't matter, but you're inconsistent about moving vs copying range


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:706
+    Diag NewDiag;
+    // Copy all fields but Notes, Fixes, Name and Tags.
+    static_cast<DiagBase &>(NewDiag) = static_cast<DiagBase>(D);
----------------
reasoning about the fields you're *not* rewriting feels fragile. (Here and in TranslateFix).

Consider copying the whole object and mutating in place (`bool TranslateDiag(Diag&)` together with `erase_if`)


================
Comment at: clang-tools-extra/clangd/Preamble.h:162
+  /// Returns diag locations for Modified contents.
+  llvm::ArrayRef<Diag> patchedDiags() const { return PatchedDiags; }
   static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";
----------------
nit: blank line after


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:820
+  {
+    // Note is also dropped if diag is gone.
+    Annotations Code(R"(
----------------
i'm confused about what this comment is getting at - a note without a diag is not even representable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096