[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Headers.h:64
+// Important: The graph generated by those callbacks might contain cycles and
+// self edges.
 using IncludeGraph = llvm::StringMap<IncludeGraphNode>;
----------------
And multi-edges too, right? Even though they're not useful.


================
Comment at: unittests/clangd/IndexActionTests.cpp:168
+  std::string MainFilePath = testPath("main.cpp");
+  std::pair<std::string, std::string> CommonHeader = {testPath("common.h"),
+                                                      R"cpp(
----------------
Maybe have two variables instead? Would format better and `HeaderPath` is arguably more readable than `Header.first`


================
Comment at: unittests/clangd/IndexActionTests.cpp:211
+  std::string MainCode = R"cpp(
+                                                      #ifndef FOO
+                                                      #define FOO "main.cpp"
----------------
Is there a way to make this format nicer?


================
Comment at: unittests/clangd/IndexActionTests.cpp:225
+  ASSERT_TRUE(IndexFile.Sources);
+  auto Nodes = toMap(*IndexFile.Sources);
+
----------------
Why not `auto Nodes = toMap(*runIndexingAction(MainFilePath).Sources)`?
Optional has an assertion, so we'll catch empty results anyway, no need to make the test more verbose


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999