[PATCH] D57508: [clangd] Enable include insertion for dynamic index

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 4 03:14:07 PST 2019

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?

  rCTE Clang Tools Extra



More information about the cfe-commits mailing list