[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/index/Background.cpp:184
+// We keep only the node "Path" and its edges.
+IncludeGraph getSubGraph(llvm::StringRef Path, const IncludeGraph &FullGraph) {
+  IncludeGraph IG;
----------------
We should make the function static or put it into the anonymous namespace.
While here the same should be done for `URIToFileCache`.


================
Comment at: clangd/index/Background.cpp:187
+
+  std::string FileURI = URI::create(Path).toString();
+  auto Entry = IG.try_emplace(FileURI).first;
----------------
NIT: Maybe accept a URI directly? E.g. if we ever have clients that already store a URI they won't need to do double conversions.


================
Comment at: clangd/index/Background.cpp:194
+  for (auto &Include : Node.DirectIncludes) {
+    auto I = IG.try_emplace(Include).first;
+    I->getValue().URI = I->getKey();
----------------
Maybe add a comment mentioning we do this merely to intern the strings?
This might be hard to follow on the first read.


================
Comment at: clangd/index/Background.cpp:384
 
   log("Indexed {0} ({1} symbols, {2} refs)", Inputs.CompileCommand.Filename,
+      Index.Symbols->size(), Index.Refs->numRefs());
----------------
Maybe also log the stats from the include graph?
It's probably less useful than symbols and refs count, but still...


================
Comment at: clangd/index/Background.cpp:387
+  SPAN_ATTACH(Tracer, "symbols", int(Index.Symbols->size()));
+  SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
 
----------------
Same here, maybe also attach some stats from the include graph?


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:192
+                        [&](llvm::StringRef) { return &MSS; });
+    CDB.setCompileCommand(testPath("root"), Cmd);
+    ASSERT_TRUE(Idx.blockUntilIdleForTest());
----------------
This sets a compile command for a directory. Should it be `"root/A.cc"` or am I missing something?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062