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