[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/Diagnostics.cpp:78
+// offsets when displaying that information to users.
+Position toOneBased(Position P) {
+  ++P.line;
----------------
Could we avoid introducing a function that breaks the invariant of a type?
Having a function to print `Position` differently would be a much better fit.


================
Comment at: clangd/Diagnostics.cpp:225
+    const auto &Inc = D.IncludeStack[I];
+    OS << "\nIn file included from: " << Inc.first << ':'
+       << toOneBased(Inc.second);
----------------
WDYT about changing this to a shorter form?
```
include stack:

./project/foo.h:312
/usr/include/vector:344
```
Note that it does not mention column numbers as they aren't useful and is shorter.
Since we're already not 100% aligned with clang, why not have a more concise representation?


================
Comment at: unittests/clangd/DiagnosticsTests.cpp:739
+                           {"/clangd-test/a.h", pos(0, 9)},
+                           {"/clangd-test/c.h", pos(3, 6)}})));
+}
----------------
So the last location in the include stack is actually the original location of an error.
This is non-obvious design. Maybe move this into a separate field?


================
Comment at: unittests/clangd/TestTU.h:51
 
+  // Absolute path and contents of each file.
+  std::vector<std::pair<Path, std::string>> AdditionalFiles;
----------------
Maybe use relative paths?
This would be consistent with `Filename` and `HeaderFilename`


================
Comment at: unittests/clangd/TestTU.h:52
+  // Absolute path and contents of each file.
+  std::vector<std::pair<Path, std::string>> AdditionalFiles;
+
----------------
Why not `StringMap</*Content*/std::string>`?
This would be consistent with `buildTestFS` and disallow adding duplicates.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302