[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 10:47:35 PDT 2019


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





More information about the cfe-commits mailing list