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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 30 06:01:15 PDT 2018


sammccall accepted this revision.
sammccall 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;
----------------
ioeric wrote:
> 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.
In that case, do we need to update IndexedFileDigests here too?

(I just realized there's an edge case: what if there are no symbols or references in a file? but I'm not sure if this is different between mainfile and headers, either)


================
Comment at: clangd/index/Background.h:55
 
+  using FileDigest = decltype(llvm::SHA1::hash({}));
+  using FileDigests = llvm::StringMap<FileDigest>;
----------------
ioeric wrote:
> 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?
oops, true. I'd suggest splitting the difference and keeping FileDigest public and dropping the FileDigests alias altogether, but up to you.


================
Comment at: clangd/index/FileIndex.cpp:140
+  }
+  case DuplicateHandling::PickOne: {
+    for (const auto &Slab : SymbolSlabs)
----------------
ioeric wrote:
> 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. 
Thanks! The FIXME is still warranted, to remove deduping from Dex.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53433





More information about the cfe-commits mailing list