[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice! Just readability/wording nits



================
Comment at: clangd/Diagnostics.cpp:463
+  // header.
+  auto ShouldAddDiag = [this](const Diag &D) {
+    if (mentionsMainFile(D))
----------------
ilya-biryukov wrote:
> Could you inline `ShouldAddDiag` into its single callsite?
Does this improve readability over (inline) `D.Severity >= DiagnosticsEngine::Error`?


================
Comment at: clangd/Diagnostics.cpp:137
+  N.File = FE->getName();
+  N.Message = "Error origin";
+  N.Range = diagnosticRange(Info, LangOpts);
----------------
Clang tends to use a phrase ending in "here" for such notes.
Suggest "error occurred here"


================
Comment at: clangd/Diagnostics.cpp:137
+  N.File = FE->getName();
+  N.Message = "Error origin";
+  N.Range = diagnosticRange(Info, LangOpts);
----------------
sammccall wrote:
> Clang tends to use a phrase ending in "here" for such notes.
> Suggest "error occurred here"
message should not be capitalized (capitalization will be added later if needed)


================
Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+                           llvm::sys::path::filename(FE->getName())) +
----------------
Include is not a noun. "included file"?


================
Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+                           llvm::sys::path::filename(FE->getName())) +
----------------
sammccall wrote:
> Include is not a noun. "included file"?
"Error" is the severity, which appears elsewhere in LSP. (Other messages do not include it)


================
Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+                           llvm::sys::path::filename(FE->getName())) +
----------------
sammccall wrote:
> sammccall wrote:
> > Include is not a noun. "included file"?
> "Error" is the severity, which appears elsewhere in LSP. (Other messages do not include it)
message should not be capitalized


================
Comment at: clangd/Diagnostics.cpp:142
+  D.Message = (llvm::Twine("Error in include ",
+                           llvm::sys::path::filename(FE->getName())) +
+               ": " + D.Message)
----------------
why are we including the filename here?

it obscures the error message, and will be available via the note.


================
Comment at: clangd/Diagnostics.h:141
   llvm::Optional<Diag> LastDiag;
+  /// Stores lines of the includes containing a diagnostic.
+  llvm::DenseSet<int> IncludeHasDiag;
----------------
nit: "containing an error" (and variable name)

IncludeLinesWithErrors might be a clearer name


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302