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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
