sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks for the split & refactoring! This looks nice. Just nits, plus some thoughts on the test. ================ Comment at: clangd/index/Index.cpp:11 #include "Index.h" - +#include "clang/Sema/CodeCompleteConsumer.h" #include "llvm/Support/SHA1.h" ---------------- changes in this file can now be reverted I think ================ Comment at: clangd/index/Index.h:128 + /// \brief Matches symbols in the index fuzzily and applies \p Callback on + /// each matched symbol. + /// ---------------- nit: "... before returning". Just to be explicit that this is a synchronous API. ================ Comment at: clangd/index/Index.h:130 + /// + /// Returns true if all candidates are matched. + virtual bool ---------------- This is a little vague - Returns true if the result list is complete, false if it was truncated due to MaxCandidateCount? Might be clearer to invert it - Returns true if the result list was truncated - up to you. ================ Comment at: clangd/index/Index.h:132 + virtual bool + fuzzyFind(const FuzzyFindRequest &Req, + std::function<void(const Symbol &)> Callback) const = 0; ---------------- context patch has landed, so this should now take const Context& as first parameter ================ Comment at: clangd/index/Index.h:136 + // FIXME: add interfaces for more index use cases: + // - Symbol getSymbolInfo(llvm::StringRef USR); + // - getAllOccurrences(llvm::StringRef USR); ---------------- USRs will rather be SymbolIDs ================ Comment at: clangd/index/MemIndex.cpp:38 + // Find all symbols that contain the query, igoring cases. + // FIXME: use better matching algorithm, e.g. fuzzy matcher. + if (StringRef(StringRef(Sym->QualifiedName).lower()) ---------------- you can easily do this now - it shouldn't even be more lines of code. Up to you of course. ================ Comment at: unittests/clangd/IndexTests.cpp:29 + +struct CountedSymbolSlab { + CountedSymbolSlab() = delete; ---------------- nit: given the name, I actually think inheriting from Slab might be clearer. Though I'm not sure we actually need this, see `weak_ptr` part of next comment. ================ Comment at: unittests/clangd/IndexTests.cpp:41 + +class MemIndexTest : public ::testing::Test { +protected: ---------------- This is a nice test, the one weakness I see is it's hard to read the cases in isolation as there's quite a lot of action-at-a-distance. I think we can eliminate the base class without adding much boilerplate, which would make the tests more self-contained: - `Index` can be owned by the test - `match()` is good, but moving `Index.build()` call into the test, and accepting `Index` as a param means this interaction is more obvious and `match` can be a free function. - `CountedSymbolSlab` can more directly be replaced by a `weak_ptr`, which can tell you whether a `shared_ptr` is alive - `generateNumSymbols` (already) doesn't need to be a member. Having it optionally emit a weak_ptr to its return value would simplify the tests a little. So MemIndexSymbolsRecycled would look something like Index I; std::weak_ptr<vector<const Symbol*>> Symbols; I.build(generateNumSymbols(0, 10), &Symbols); EXPECT_THAT(match(I, Req), ElementsAre("7")); EXPECT_FALSE(Symbols.expired()); I.build(generateNumSymbols(0,0)); EXPECT_TRUE(Symbols.expired()); WDYT? ================ Comment at: unittests/clangd/IndexTests.cpp:62 + Slab->Slab.insert(symbol(std::to_string(i))); + auto *Symbols = new std::vector<const Symbol *>(); + ---------------- This is a leak - the returned shared_ptr doesn't own this vector. (I'm actually fine with this for simplicity if it's documented - but we might be running leak checkers that complain about it?) In principle you need to make_shared a struct containing both the vector and the slab. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits