ilya-biryukov added a comment.
Thanks for changes. A few more comments.
================
Comment at: clangd/ClangdUnit.cpp:259
+// Generates a mapping from start of an include directive to its range.
+std::map<Position, Range>
+positionToRangeMap(const std::vector<Inclusion> &MainFileIncludes) {
----------------
Could we binary-search in a sorted `vector<Inclusion>` with a custom comparator
instead?
================
Comment at: clangd/ClangdUnit.cpp:411
+ MaxDiagsPerIncludeDirective) {
+ ++NumberOfDiagsAtLine[D.IncludeDirective->line];
+ return true;
----------------
The function name suggests it does not have side-effects.
Maybe handle the update and query of `NumberOfDiagstAtLine` outside this
function instead?
================
Comment at: clangd/Diagnostics.cpp:396
+ for (llvm::StringRef Inc : IncludeStack)
+ OS << "In file included from: " << Inc << ":\n";
+ }
----------------
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.
================
Comment at: clangd/Diagnostics.cpp:419
- else
- vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
- LastDiag->Message);
----------------
We can still drop diagnostics at some point, right?
Could we move the log statement in a different point?
================
Comment at: clangd/Diagnostics.h:87
+ /// directive, otherwise None.
+ llvm::Optional<Position> IncludeDirective = llvm::None;
};
----------------
Could we avoid changing this interface?
We have enough information to fill the corresponding range and message when
collecting diagnostics inside our implementation of clang's
`DiagnosticConsumer`, there's really no point in carrying this information in
this struct.
If that means intermediate structures stored in our consumer, so be it, it's a
private implementation detail. `Diag` is a public interface, so keeping it
simpler is valuable.
================
Comment at: unittests/clangd/DiagnosticsTests.cpp:773
+ UnorderedElementsAre(
+ Diag(Main.range(), "/clangd-test/a.h:2:44: C++ requires a "
+ "type specifier for all declarations")));
----------------
NIT: maybe use raw string literals to keep the message a one-liner?
================
Comment at: unittests/clangd/DiagnosticsTests.cpp:779
} // namespace clangd
} // namespace clang
----------------
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...
```
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
