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

[Eric Liu via Phabricator via cfe-commits]

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