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