[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:23
+// Collects the nodes and edges of include graph during indexing action.
+struct IncludeGraphCollector : public PPCallbacks {
+public:
----------------
Could we add an assertion that the final graph does not contain uninitialized nodes?


================
Comment at: clangd/index/IndexAction.cpp:28
+
+  // Populates the node for a new include.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
----------------
Maybe expand a bit on what a node is? I.e. mention file digests, URIs, etc.


================
Comment at: clangd/index/IndexAction.cpp:33
+    // We only need to process each file once. So we don't care about anything
+    // but enteries.
+    if (Reason != FileChangeReason::EnterFile)
----------------
s/enteries/entries 


================
Comment at: clangd/index/IndexAction.cpp:44
+
+    auto &Node = I->getValue();
+    if (auto Digest = digestFile(SM, FileID))
----------------
Is there a way to check the node has already been populated? Can we do this and avoid recomputing the hashes for the same file/assert they're the same in assertion mode?
E.g. by looking at `URI.empty()`?


================
Comment at: clangd/index/IndexAction.cpp:45
+    auto &Node = I->getValue();
+    if (auto Digest = digestFile(SM, FileID))
+      Node.Digest = std::move(*Digest);
----------------
What happens if we can't compute a digest for a file? Are we left with an uninitialized memory?


================
Comment at: clangd/index/IndexAction.cpp:63
+
+    auto IncludingURI =
+        URIFromFileEntry(SM.getFileEntryForID(SM.getFileID(HashLoc)));
----------------
Maybe convert both URIs before adding anything to `IG`?
To make sure we don't change `IG` unless both URIs can be parsed


================
Comment at: clangd/index/IndexAction.cpp:72
+
+#ifndef NDEBUG
+  // Sanity check to ensure we have already populated a skipped file.
----------------
Maybe move the if block into the function body? To make sure we're be producing compiler errors on changes to the base function signature regardless of the compile configuration


================
Comment at: clangd/index/IndexAction.cpp:109
     CI.getPreprocessor().addCommentHandler(PragmaHandler.get());
+    if (IncludeGraphCallback != nullptr)
+      CI.getPreprocessor().addPPCallbacks(
----------------
NIT: maybe remove `!= nullptr`? the callback is a function, not a pointer, so `nullptr` might be a bit confusing


================
Comment at: clangd/index/IndexAction.cpp:133
       RefsCallback(Collector->takeRefs());
+    if (IncludeGraphCallback != nullptr)
+      IncludeGraphCallback(std::move(IG));
----------------
NIT: maybe remove `!= nullptr`?


================
Comment at: unittests/clangd/IndexActionTests.cpp:79
+      {testPath("level1_1.h"), "#include \"level2_1.h\""},
+      {testPath("level1_2.h"), "#include \"level2_2.h\""},
+      {testPath("level1_3.h"), "#include \"level2_3.h\""}};
----------------
Why 3 headers on each level? 1 or 2, would probably read a but simpler.


================
Comment at: unittests/clangd/IndexActionTests.cpp:104
+  EXPECT_TRUE(Main.IsTU);
+  EXPECT_EQ(Main.URI.data(), IG.find(PathToURI(MainFilePath))->getKeyData());
+
----------------
Directly looking at the graph to capture the include structure might be more straightforward, e.g. 
```
EXPECT_THAT(graph, 
 UnorderedElementsAre(Pair(Main, IncludersAre("level1", ....)))
```
This won't work on `StringMap`, but converting to `std::map<string, node>`  should be simple. Other invariants can be tested separately (e.g. values of `IsTU`)
Maybe try that?


================
Comment at: unittests/clangd/IndexActionTests.cpp:126
+}
+
+} // namespace
----------------
Could we also test a few corner cases?

- Self-includes (with and without include guards).
- Skipped files (multiple includes of the same file with `#pragma once` or include guards)



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999