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