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