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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:251
+  auto Res =
+      std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc,
+                       [](const Inclusion &LHS, const Inclusion &RHS) {
----------------
NIT: use `llvm::lower_bound`


================
Comment at: clangd/ClangdUnit.cpp:252
+      std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc,
+                       [](const Inclusion &LHS, const Inclusion &RHS) {
+                         return LHS.R.start.line < RHS.R.start.line;
----------------
NIT: `lower_bound` also works when you work on a different type, no need for the temporary variable.
```
lower_bound(MainFileIncludes…, [](const Inclusion &L, const Position &Pos) {
  return L.start.line < Pos.line;
})
```

(the order of lamda parameters might need to be reversed, I always forget what's the correct one)


================
Comment at: clangd/ClangdUnit.cpp:255
+                       });
+  return Res->R;
+}
----------------
NIT: add a sanity check that the include was actually there: `assert(Res->R == Pos)` 


================
Comment at: clangd/Diagnostics.cpp:55
+  }
+  if (!IncludeStack.empty()) {
+    D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
----------------
reduce nesting by inverting the condition:
```
if (IncludeStack.empty())
  return;
// rest of the code
```


================
Comment at: clangd/Diagnostics.cpp:205
+      if (I != E - 1)
+        OS << "In file included from: ";
+      OS << D.IncludeStack[I] << ": ";
----------------
Could we get the message first and the include stack later?
The first line is often showed in various lists and having `In file included from` there is not very helpful.


================
Comment at: clangd/Diagnostics.cpp:463
+  // header.
+  auto ShouldAddDiag = [this](const Diag &D) {
+    if (mentionsMainFile(D))
----------------
Could you inline `ShouldAddDiag` into its single callsite?


================
Comment at: clangd/Diagnostics.h:87
+  /// file is in front.
+  std::vector<std::string> IncludeStack;
 };
----------------
Could you store `pair</*Filename*/string,  Position>` instead?
Would make it simpler to adapt D60267 to this change.


================
Comment at: clangd/Diagnostics.h:131
   llvm::Optional<Diag> LastDiag;
+  // Counts diagnostics coming from a header in the main file.
+  llvm::DenseMap<int, int> NumberOfDiagsAtLine;
----------------
NIT: use tripple slash comments


================
Comment at: clangd/Diagnostics.h:132
+  // Counts diagnostics coming from a header in the main file.
+  llvm::DenseMap<int, int> NumberOfDiagsAtLine;
 };
----------------
NIT: the name makes me believe this does a different thing. Maybe mention "include" in the name to make it less confusing?
Something like `DiagsInsideIncludes` would work.

Another NIT: we don't need a map anymore: `DenseSet<int>` should be enough


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:54
+    return false;
+  for (size_t I = 0, E = IncludeStack.size(); I < E; ++I)
+    if (IncludeStack[I] != arg.IncludeStack[I])
----------------
NIT: maybe simplify to `std::equal(IncludeStack.begin(), IncludeStack.end(), arg.IncludeStack.begin())`


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:620
 
+ParsedAST build(const std::string &MainFile,
+                const llvm::StringMap<std::string> &Files) {
----------------
We were discussing adding the extra files to `TestTU` instead.
Any reason this did not work out?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59302/new/

https://reviews.llvm.org/D59302