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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clangd/Diagnostics.cpp:396
+          for (llvm::StringRef Inc : IncludeStack)
+            OS << "In file included from: " << Inc << ":\n";
+        }
----------------
ilya-biryukov wrote:
> 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.
> 
SG, delaying serialization to LSP layer then.


================
Comment at: clangd/Diagnostics.cpp:372
+    auto IncludeLocation = Info.getSourceManager()
+                               .getPresumedLoc(Info.getLocation(),
+                                               /*UseLineDirectives=*/false)
----------------
ilya-biryukov wrote:
> 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?
It was the way `DiagnosticRenderer` emitted include stacks, I had just copied it over.
Changing with `SourceManger::getIncludeLoc`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302