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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:405
 
-  std::vector<Diag> Diags = ASTDiags.take();
+  auto ShouldSurfaceDiag = [](const Diag &D) {
+    if (D.Severity == DiagnosticsEngine::Level::Error ||
----------------
The name suggests we filter **all** diagnostics based on this helper.
Maybe add rename to something more specific?

E.g. `IsErrorOrFatal` or `IsImporantHeaderDiagnostic`...


================
Comment at: clangd/ClangdUnit.cpp:408
+        D.Severity == DiagnosticsEngine::Level::Fatal)
+      return true;
+    return false;
----------------
NIT: simplify to 
```
return D.Severity == DiagnosticsEngine::Level::Error
    || D.Severity == DiagnosticsEngine::Level::Fatal
```


================
Comment at: clangd/ClangdUnit.cpp:414
+  llvm::DenseMap<int, int> NumberOfDiagsAtLine;
+  auto TryAddDiag = [&Diags, &NumberOfDiagsAtLine](const Diag &D) {
+    if (++NumberOfDiagsAtLine[D.Range.start.line] > 1) {
----------------
NIT: rename to `AddDiagnosticFromInclude` or something similar, but more specific than the current name


================
Comment at: clangd/ClangdUnit.cpp:425
+      Diags.push_back(D);
+    else if (ShouldSurfaceDiag(D))
+      TryAddDiag(D);
----------------
Why not merge the `ShouldSurfaceDiag` and `TryAddDiag` into a single function and handle all the complexities there?
This would simplify the client code, it would be as simple as 
```
else
 AddDiagnosticFromInclude(D)
```


================
Comment at: clangd/ClangdUnit.cpp:434
+  if (Preamble) {
+    for (const Diag &D : Preamble->Diags) {
+      if (mentionsMainFile(D))
----------------
Can we do this in `StoreDiags::flushLastDiag`?
All code handling the diagnostics lives there and it has all the information necessary to map to the included line.


================
Comment at: clangd/ClangdUnit.cpp:445
+  // same line.
+  for (auto &D : Diags) {
+    if (!mentionsMainFile(D)) {
----------------
This extra complexity does not seem to be worth it after all.
I'd suggest to remove this (at least from the first version of the file), even though I was the one who proposed it.

Still think it's valuable, but the patch is complicated enough as it is, keeping it simpler seems to be more important at this point.


================
Comment at: clangd/Diagnostics.cpp:396
+          for (llvm::StringRef Inc : IncludeStack)
+            OS << "In file included from: " << Inc << ":\n";
+        }
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > NIT: could we reuse the function from clang that does the same thing?
> > 
> > The code here is pretty simple, though, so writing it here is fine. Just wanted to make sure we considered this option and found it's easier to redo this work ourselves.
> there is `TextDiagnostic` which prints the desired output expect the fact that it also prints the first line saying the header was included in main file. Therefore, I didn't make use of it since we decided that was not very useful for our use case. And it requires inheriting from `DiagnosticRenderer` which was requiring too much boiler plate code just to get this functionality so instead I've chosen doing it like this.
> 
> Can fallback to `TextDiagnostic` if you believe showing `Included in mainfile.cc:x:y:` at the beginning of the diagnostic won't be annoying.
LG, it's not too hard to reconstruct the same output.
Note that D60267 starts outputting notes in a structured way, using the `RelatedInformation` struct from the LSP.

We should eventually add the include stack into related information too. With that in mind, we should probably add the include stack as a new field to the struct we use for diagnostics.



================
Comment at: clangd/Diagnostics.cpp:371
     FillDiagBase(*LastDiag);
+    auto IncludeLocation = Info.getSourceManager()
+                               .getPresumedLoc(Info.getLocation(),
----------------
That's a lot of code, could we extract this into a separate function?


================
Comment at: clangd/Diagnostics.cpp:372
+    auto IncludeLocation = Info.getSourceManager()
+                               .getPresumedLoc(Info.getLocation(),
+                                               /*UseLineDirectives=*/false)
----------------
Why use `getPresumedLoc`? Making clangd sensitive to pp directives that rename file or change line numbers does not make any sense.

Could you also add tests for that?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302