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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
