ioeric added inline comments. Herald added a project: clang.
================ Comment at: clangd/ClangdUnit.cpp:100 + : File(File), ParsedCallback(ParsedCallback), + IWYUHandler(collectIWYUHeaderMaps(&CanonIncludes)) {} ---------------- Does this have to own the `IWYUHandler`? Could we create one when `getCommentHandler` is called? ================ Comment at: clangd/ClangdUnit.cpp:125 + CommentHandler *getCommentHandler() override { return IWYUHandler.get(); } + ---------------- This looks like a memory leak? `release()`? ================ Comment at: clangd/ClangdUnit.cpp:338 + // Do we really care about IWYU pragmas inside the file rather than the + // preamble? ---------------- I think so? A header file (with IWYU pragma) can also be the main file. ================ Comment at: unittests/clangd/CodeCompleteTests.cpp:2287 +TEST(CompletionTest, DynamicIndexIncludeInsertion) { + MockFSProvider FS; ---------------- nit: maybe put this close to test cases that involve preamble. e.g. IndexSuppressesPreambleCompletions ================ Comment at: unittests/clangd/CodeCompleteTests.cpp:2301 + )cpp"; + constexpr const char *FileContent(R"cpp( + #include "foo_header.h" ---------------- nit: just use `std::string` for readability. ================ Comment at: unittests/clangd/FileIndexTests.cpp:207 + M, "f", + "// IWYU pragma: private, include <the/good/header.h>\nclass string {};"); ---------------- Are we sure the comment will be included in the preamble if there is no #include block? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57508/new/ https://reviews.llvm.org/D57508 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits