[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/index/IndexAction.cpp:31
+  // Populates the following fields of the corresponding node for a new include:
+  // - Digest -> SHA1 hash of the file.
+  // - IsTU -> true if the file is the main file the indexing action has been
----------------
No need to duplicate the comments here, one can always look at the struct definition.
Mentioning it populates everything except the include structure should be enough.


================
Comment at: clangd/index/IndexAction.cpp:36
+  // We perform this population in FileChanged rather than InclusionDirective to
+  // get rid of possible IO, since we have FileID in this callback we can use it
+  // to query SourceManager and use the cached file if already read.
----------------
No need to mention IO, something like the following should convey the same message: `we cannot populate the fields in InclusionDirective because it does not have access to the contents of the target file`


================
Comment at: unittests/clangd/IndexActionTests.cpp:28
+std::string PathToURI(llvm::StringRef Path) {
+  return URI::create(Path).toString();
+}
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe inline this? This looks simple enough.
> yeah but repeating all of that over and over again looks really ugly.
Maybe call it `toUri` to make it even shorther then? 
Or at least `pathToURI` to follow the naming conventions.


================
Comment at: unittests/clangd/IndexActionTests.cpp:52
+    const auto &Node = IndexFile.Sources->lookup(URI);
+    EXPECT_EQ(Node.URI.data(), IndexFile.Sources->find(URI)->getKeyData());
+  }
----------------
NIT: add a comment that uninitialized nodes will have an empty URI?


================
Comment at: unittests/clangd/IndexActionTests.cpp:109
+
+  IndexFileIn IndexFile = runIndexingAction(MainFilePath);
+  ASSERT_TRUE(IndexFile.Sources);
----------------
Do we ever use anything but the include graph?
Maybe create a helper function that return `vector<pair<string, IncludeGraphNode>>` (or even `map<string, IncludeGraphNode>`) to avoid repeating the conversion code in every test?


================
Comment at: unittests/clangd/IndexActionTests.cpp:128
+
+  checkNodesAreInitialized(
+      IndexFile, {MainFilePath, Level1Header.first, Level2Header.first});
----------------
Could we move these checks into the body of `runIndexingAction` to avoid repeating them in every test?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999