[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:12
 
+llvm::Optional<std::string> URIFromFileEntry(const FileEntry *File) {
+  if (!File)
----------------
NIT: maybe call it `toURI`?


================
Comment at: clangd/index/IndexAction.cpp:23
+// Collects the nodes and edges of include graph during indexing action.
+// Important: The graph generated by those callbacks might contain cycles and
+// self edges.
----------------
I think this comment would also be useful on the `IncludeGraph` definition itself, most of the readers won't see the code building the graph


================
Comment at: clangd/index/IndexAction.cpp:34
+  //   started on.
+  // - URI -> Reference to the key for this node in the include graph.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
----------------
NIT: maybe add a note mentioning why we have to do this separately from `InclusionDirective`?


================
Comment at: clangd/index/IndexAction.cpp:152
+      for (const auto &Node : IG)
+        assert(Node.getKeyData() == Node.getValue().URI.data());
+#endif
----------------
NIT: maybe add a comment that we are checking for the absence of the uninitialized values here?


================
Comment at: unittests/clangd/IndexActionTests.cpp:28
+std::string PathToURI(llvm::StringRef Path) {
+  return URI::create(Path).toString();
+}
----------------
Maybe inline this? This looks simple enough.


================
Comment at: unittests/clangd/IndexActionTests.cpp:32
+MATCHER(IsTU, "") {
+  const IncludeGraphNode &Node = arg;
+  return Node.IsTU;
----------------
Why not simply return `arg.IsTU`?


================
Comment at: unittests/clangd/IndexActionTests.cpp:38
+  const IncludeGraphNode &Node = arg;
+  return Node.Digest == Digest;
+}
----------------
Why not `return arg.Digest == Digest`?


================
Comment at: unittests/clangd/IndexActionTests.cpp:88
+  IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem;
+  SymbolSlab Symbols;
+  RefSlab Refs;
----------------
Maybe return all outputs in `runIndexingAction` in a dedicated struct?
To make the inputs for the tests more explicit.

Unless this has also been copied from some other test, in which case you could argue it's consistent with how we did things before.


================
Comment at: unittests/clangd/IndexActionTests.cpp:124
+
+  for (const auto &Path :
+       {MainFilePath, Level1Header.first, Level2Header.first}) {
----------------
NIT: Maybe use `StringRef`? i.e. `for (StringRef Path : ...)`


================
Comment at: unittests/clangd/IndexActionTests.cpp:149
+  runIndexingAction(MainFilePath);
+  std::vector<std::pair<std::string, IncludeGraphNode>> IGFlattened;
+  for (auto &I : IG)
----------------
Maybe directly store `map<string, IncludeGraphNode>` to avoid the flattenning code in every test?


================
Comment at: unittests/clangd/IndexActionTests.cpp:162
+
+  for (const auto &Path : {MainFilePath, Header.first}) {
+    auto URI = PathToURI(Path);
----------------
Maybe move to the common code? Repeating these in every test does not seem ideal, it's a common invariant.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999