[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/ParsedAST.cpp:678
+    if (Preamble) {
+      auto PDiags = Patch->patchedDiags();
+      Diags->insert(Diags->end(), PDiags.begin(), PDiags.end());
----------------
llvm::append_range(Diags, Patch->patchedDiags()) ?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:373
   SP.Includes = std::move(Includes.MainFileIncludes);
+  for (auto Pos = Contents.find('\n'); Pos != Contents.npos;
+       Contents = Contents.substr(Pos + 1), Pos = Contents.find('\n')) {
----------------
llvm::append_range(SP.Lines, llvm::split(Contents, "\n")) ?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:639
 
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {
----------------
nit: comment sense is the opposite of the function name

maybe say why this is an important property, or give the function a higher-level name?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:640
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {
+  int Line = D.Range.start.line;
----------------
this function + translateDiag look like a validate/parse pair that need to be kept in sync.

they could be a single `translateDiag()` function that could fail, instead.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+  bool NotePointsToOutside = llvm::any_of(D.Notes, [&](const Note &N) {
+    return N.File == D.File &&
+           (N.Range.start.line != Line || N.Range.end.line != Line);
----------------
this check looks dubious to me:
 - `File` is a human-readable string with ill-defined format, comparing them for equality isn't safe, you want InsideMainFile instead
 - if the note points to another file, then it's not that translation is impossible, but rather no translation is needed
 - if the note points to the same file, then why can't we apply the same translation?

I think this should rather be something like:
```
bool translateDiag(DiagBase& D, bool IsMainDiag) {
  if (IsMainDiag) {
    for (const auto& N : cast<Diag>(D).Notes) {
      if (!translateDiag(N, /*IsMainDiag=*/false))
        return false;
    }
  }
  if (D.InsideMainFile) {
    // attempt patching range of D itself
  }
}
```



================
Comment at: clang-tools-extra/clangd/Preamble.cpp:680
+static llvm::DenseMap<int, llvm::SmallVector<const Diag *>>
+mapDiagsToLines(llvm::ArrayRef<Diag> Diags) {
+  llvm::DenseMap<int, llvm::SmallVector<const Diag *>> LineToDiags;
----------------
nit: name seems backwards (but as noted below I think this function can go away entirely)


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:705
+// same spelling.
+static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
+                                    const ScannedPreamble &BaselineScan,
----------------
If I'm understanding the algorithm right:
 - we make a map of line content => original line number
 - we iterate through each line of the modified preamble, and:
    - find the corresponding original line
    - look for diagnostics to translate

Why not loop over diagnostics instead of lines in the modified preamble? There should be a lot fewer, and you avoid the lines-to-diags map.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:707
+                                    const ScannedPreamble &BaselineScan,
+                                    const ScannedPreamble &ModifiedScan) {
+  std::vector<Diag> PatchedDiags;
----------------
large preamble with no diagnostics seems pretty plausible, maybe bail out early to avoid building the content map?


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:795
     // Baseline for exclusion.
-    llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
-                   const Inclusion *>
-        ExistingIncludes;
-    for (const auto &Inc : Baseline.Includes.MainFileIncludes)
-      ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
-    // There might be includes coming from disabled regions, record these for
-    // exclusion too. note that we don't have resolved paths for those.
-    for (const auto &Inc : BaselineScan->Includes)
-      ExistingIncludes.try_emplace({Inc.Directive, Inc.Written});
+    auto ExistingIncludes =
+        getExistingIncludes(*BaselineScan, Baseline.Includes.MainFileIncludes);
----------------
I think this change and the IncludeKey class can be reverted now?


================
Comment at: clang-tools-extra/clangd/Preamble.h:161
 
+  /// Returns diag locations for Modified contents, only contains diags attached
+  /// to an #include or #define directive.
----------------
the stuff about `#include`/`#define` doesn't seem to correspond to anything in the code (obsolete?)
and below


================
Comment at: clang-tools-extra/clangd/Preamble.h:175
   std::string PatchFileName;
-  /// Includes that are present in both \p Baseline and \p Modified. Used for
-  /// patching includes of baseline preamble.
+  // Includes that are present in both \p Baseline and \p Modified. Used for
+  // patching includes of baseline preamble.
----------------
if you want to undoxygen this comment, also remove `\p`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143096