ioeric added inline comments.
================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:137 + BoostingIterators.push_back( + createBoost(create(It->second), P.second + 10)); + } ---------------- Could you comment on `P.second + 10` here? It sounds like a lot of boost. ================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:157 + const size_t ItemsToRetrieve = + getRetrievalItemsMultiplier() * Req.MaxCandidateCount; auto Root = createLimit(move(QueryIterator), ItemsToRetrieve); ---------------- Again, the multiplier change seems irrelevant in this patch. ================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:163 + // Sort items using boosting score as the key. + std::sort(begin(SymbolDocIDs), end(SymbolDocIDs), + [](const std::pair<DocID, float> &LHS, ---------------- Shouldn't we sort them by `Quality * boost`? ================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:64 -private: + /// For fuzzyFind() Dex retrieves getRetrievalItemsMultiplier() more items + /// than requested via Req.MaxCandidateCount in the first stage of filtering. ---------------- kbobyrev wrote: > ioeric wrote: > > Why are values of multipliers interesting to users? Could these be > > implementation details in the cpp file? > Actually, my understanding is that users might want to have full access to > the multipliers at some point to control the performance/quality ratio. > > And it's also useful for the tests: otherwise the last one would have to > hard-code number of generated symbols to ensure only boosted ones are in the > returned list. It would have to be updated each time these internal > multipliers are and we might update them often/make logic less clear (by > allowing users to control these parameters). > > What do you think? I'm not sure if users can usefully control this multipliers without understanding the whole implementation. Do we have a use case for these APIs now? If not, I'd suggesting removing them from this patch. It also seems to be out of the scope of this patch. ================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:73 +private: +private: mutable std::mutex Mutex; ---------------- Double `private`? ================ Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:87 + + std::vector<std::string> URISchemes /*GUARDED BY(Mutex)*/; + ---------------- I think `URISchemes` should be initialized when creating a `DexIndex` and remain the same. So this could be `const` without mutex guard. ================ Comment at: clang-tools-extra/clangd/index/dex/Token.h:54 Scope, + /// Path to symbol declaration. + /// ---------------- As this is called `Path`, I'd try to decouple it from URIs, so that we don't need special handling of `scheme` etc in the implementation. We might want to canonicalize URI to "path" like `/file:/path/to/something/` so that we could simply treat it as normal paths, and the token could potentially handle actual actual file paths. ================ Comment at: clang-tools-extra/clangd/index/dex/Token.h:97 +/// Returns Search Token for each parent directory of given Path. Should be used +/// within the index build process. ---------------- I couldn't find implementations of these two functions in this patch? https://reviews.llvm.org/D51481 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits