[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/index/Background.cpp:311
   SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
-  // FIXME: partition the symbols by file rather than TU, to avoid duplication.
-  IndexedSymbols.update(AbsolutePath,
-                        llvm::make_unique<SymbolSlab>(std::move(Symbols)),
-                        llvm::make_unique<RefSlab>(std::move(Refs)));
-  FileHash[AbsolutePath] = Hash;
+  update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate);
+  IndexedFileDigests[AbsolutePath] = Hash;
----------------
sammccall wrote:
> It looks like you never write the FilesToUpdate hashes back to the IndexedFileDigests, so we'll always update headers?
It's updated in `update` when slabs are updated.


================
Comment at: clangd/index/Background.h:55
 
+  using FileDigest = decltype(llvm::SHA1::hash({}));
+  using FileDigests = llvm::StringMap<FileDigest>;
----------------
sammccall wrote:
> private.
These are also used in some helpers in the cpp file. I can make these private and make those helper members if you think it would be better?


================
Comment at: clangd/index/FileIndex.cpp:140
+  }
+  case DuplicateHandling::PickOne: {
+    for (const auto &Slab : SymbolSlabs)
----------------
sammccall wrote:
> since we're deduplicating above, we could deduplicate here too and remove the deduplicating logic from Dex (MemIndex gets it by mistake). That would avoid duplicated deduplication (!) in the Merge + Dex case, and seems a little cleaner.
> 
> Not something for this patch, but maybe add a FIXME.
Added the duduplication for the PickOne case. 


================
Comment at: clangd/index/SymbolCollector.cpp:442
+  auto DefLoc = MI->getDefinitionLoc();
+  // FIXME: use the result to filter out symbols.
+  shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
----------------
sammccall wrote:
> just to check - this is basically trivial to do now but would need tests etc?
> (fine to defer from this patch)
Do you mean just do this for macros for now? Yes, changes for macros should be trivial.


================
Comment at: clangd/index/SymbolCollector.h:78
+    /// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
+    std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
   };
----------------
sammccall wrote:
> I think we should explicitly test this in SymbolCollectorTests - I think it should be straightforward?
This should be trivial for macros, but we still need some refactoring to make it work for symbols (e.g. `addDeclaration` doesn't support dropping symbols). 


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:33
+      #if A
+        class A_H {};
+      #else
----------------
sammccall wrote:
> naming here seems confusing: A_H seems to refer to the header, but then what does B_H refer to?
Changed to A_CC and B_CC


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53433