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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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(
----------------
ilya-biryukov wrote:
> Maybe have two variables instead? Would format better and `HeaderPath` is arguably more readable than `Header.first`
Is it that important?


================
Comment at: unittests/clangd/IndexActionTests.cpp:225
+  ASSERT_TRUE(IndexFile.Sources);
+  auto Nodes = toMap(*IndexFile.Sources);
+
----------------
ilya-biryukov wrote:
> 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
because toMap doesn't modify the contents of the nodes(to make sure we don't change behaviour during the process), and all the strings are still referring to the keys of the graph. Therefore the graph needs to be kept alive.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999