[PATCH] D60267: [clangd] Support relatedInformation in diagnostics.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Diagnostics.cpp:185
   OS << Note.Message;
-  OS << "\n\n";
-  printDiag(OS, Main);
+  // If there's no structured link between the note and the original diagnostic
+  // then emit the main diagnostic to give context.
----------------
NIT: the comment looks open to misinterpretation, "no structured link" refers to the fact the clients don't support it, but could be read that we don't know which notes correspond to a main diagnostic.

Maybe rephrase to something link "If the client does not support structured links …"?


================
Comment at: clangd/Diagnostics.cpp:280
+      Main.relatedInformation->push_back(std::move(RelInfo));
+    }
   }
----------------
NIT: maybe call `OutFn` and return here to avoid checking for `EmitRelatedLocations` again?
Would arguably make the code simpler, although would require another call to `OutFn(Main)` outside the if branch.


================
Comment at: clangd/SourceCode.cpp:310
+  if (!F) {
     return None;
+  }
----------------
NIT: seems unrelated. Maybe revert?


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:62
+    return false;
+  if (toJSON(arg) != toJSON(LSPDiag)) {
+    *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
----------------
Maybe omit the `std::tie()`-based comparison altogether?
This would not change the semantics of the matcher, right?


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:259
+#ifdef _WIN32
+      "c:\\path\\to\\foo\\bar\\main.cpp",
+#else
----------------
maybe use `testPath()` here to avoid PP directives?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60267