hokein added a comment.
Thanks for cleaning it up! I admit that `SymbolOccurrences` is a loong name. A
few nits.
================
Comment at: clangd/index/FileIndex.cpp:120
+ auto &SymRefs = Sym.second;
+ std::sort(SymRefs.begin(), SymRefs.end());
+ std::copy(SymRefs.begin(), SymRefs.end(), back_inserter(RefsStorage));
----------------
So the sort is intended to make returned refs ordered?
================
Comment at: clangd/index/Index.h:367
+ return sizeof(*this) + Arena.getTotalMemory() +
+ sizeof(Refs.front()) * Refs.size();
}
----------------
we should be careful if `Refs` is empty. Calling `front()` on empty container
is undefined behavior.
================
Comment at: clangd/index/MemIndex.h:23
+ /// Maps from a symbol ID to all corresponding Refs.
+ using RefMap = llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>>;
----------------
This type seems not used in this patch, consider removing it or using it to
replace existing `llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>>` usage.
================
Comment at: unittests/clangd/FileIndexTests.cpp:341
+ std::vector<Ref> Results;
+ Index.refs(Request, [&Results](const Ref &O) { Results.push_back(O); });
----------------
My fault here. This is not safe, we need to store the underlying `FileURI` data.
================
Comment at: unittests/clangd/IndexTests.cpp:271
+ std::vector<Ref> Results;
+ MergedIndex->refs(Request, [&](const Ref &O) { Results.push_back(O); });
+
----------------
The same here.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51605
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
