[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:
> 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.


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:779
 } // namespace clangd
 } // namespace clang
----------------
ilya-biryukov wrote:
> Could we add a test for reaching the limit?
> Could we mention that some diagnostics were dropped in that case? Something like:
> ```
> In file included from a.h:20:1:
> error: unresolved identifier 'foo'.
> 
> And 10 more diagnostics...
> ```
There is already one, see `LimitDiagsOutsideMainFile`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302