sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
This basically looks good to go (some fixes needed but they're pretty clear I think let me know if not!) ================ Comment at: clangd/index/FileIndex.cpp:63 + auto Occurrences = Collector.takeOccurrences(); + vlog("index for AST: \n" + " symbol slab: {0} symbols, {1} bytes\n" ---------------- for this log to be useful, you might want to include the filename. You can get it from the sourcemanager on the ASTContext. ================ Comment at: clangd/index/MemIndex.cpp:35 std::unique_ptr<SymbolIndex> MemIndex::build(SymbolSlab Slab) { auto Idx = llvm::make_unique<MemIndex>(); ---------------- This is still implicitly creating an index with no occurrences. Did you mean to accept a SymbolOccurrencesSlab here? ================ Comment at: clangd/index/MemIndex.h:23 public: - /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain - /// accessible as long as `Symbols` is kept alive. - void build(std::shared_ptr<std::vector<const Symbol *>> Symbols); + using OccurrenceMap = + llvm::DenseMap<SymbolID, std::vector<const SymbolOccurrence *>>; ---------------- can you add an explanation to this? ================ Comment at: clangd/index/Merge.cpp:94 + // instead. + llvm::DenseSet<llvm::StringRef> DynamicIndexFileURIs; + Dynamic->findOccurrences(Req, [&](const SymbolOccurrence &O) { ---------------- Unfortunately this is not safe, the backing strings may not live long enough. This should be a StringSet (by value) instead. ================ Comment at: clangd/index/Merge.cpp:100 + Static->findOccurrences(Req, [&](const SymbolOccurrence &O) { + if (DynamicIndexFileURIs.find(O.Location.FileURI) != + DynamicIndexFileURIs.end()) ---------------- or just DynamicIndexFileURIs.count(O.Location.FileURI) and rely on the implicit conversion to bool ================ Comment at: clangd/index/SymbolCollector.cpp:227 +SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) { + SymbolOccurrenceKind Kind; ---------------- toOccurrenceKind isn't this just `return Roles & AllOccurrenceKinds` with some casts? (I'd put AllOccurrenceKinds into the header, but it could also be a constant here) ================ Comment at: clangd/index/SymbolCollector.h:120 + + using DeclOccurrence = std::pair<SourceLocation, index::SymbolRoleSet>; + llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences; ---------------- move these up next to ReferencedDecls and ReferencedMacros so the comment applies to them too? Similarly, move SymbolOccurrenceSlab next to the SymbolSlab::Builder? These have strong parallels. ================ Comment at: clangd/index/SymbolCollector.h:122 + llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences; + // All symbol occurrences collected from the AST, assembled on finish(). + // Only symbols declared in preamble (from #inclues) and references from the ---------------- I'm not sure this works - IIRC can use a SymbolCollector for multiple TUs, with finish() called at the end of each one. I think you need to (incrementally) build in finish(), and freeze in takeOccurrences(). ================ Comment at: unittests/clangd/TestTU.h:41 + static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code, + llvm::StringRef Filename = "") { ---------------- We've avoided adding this in the past because it's less readable. Please assign the fields separately instead. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51279 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits