[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 4 02:33:44 PDT 2018
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
More information about the cfe-commits
mailing list