Author: omtcyfz Date: Thu Sep 13 07:27:03 2018 New Revision: 342138 URL: http://llvm.org/viewvc/llvm-project?rev=342138&view=rev Log: [clangd] Cleanup FuzzyFindRequest filtering limit semantics
As discussed during D51860 review, it is better to use `llvm::Optional` here as it has clear semantics which reflect intended behavior. Reviewed By: sammccall Differential Revision: https://reviews.llvm.org/D52028 Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/clangd/FindSymbols.cpp clang-tools-extra/trunk/clangd/index/Index.cpp clang-tools-extra/trunk/clangd/index/Index.h clang-tools-extra/trunk/clangd/index/MemIndex.cpp clang-tools-extra/trunk/clangd/index/dex/Dex.cpp clang-tools-extra/trunk/test/clangd/Inputs/requests.json clang-tools-extra/trunk/unittests/clangd/DexTests.cpp clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original) +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Thu Sep 13 07:27:03 2018 @@ -1375,7 +1375,7 @@ private: // Build the query. FuzzyFindRequest Req; if (Opts.Limit) - Req.MaxCandidateCount = Opts.Limit; + Req.Limit = Opts.Limit; Req.Query = Filter->pattern(); Req.RestrictForCodeCompletion = true; Req.Scopes = QueryScopes; Modified: clang-tools-extra/trunk/clangd/FindSymbols.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FindSymbols.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/FindSymbols.cpp (original) +++ clang-tools-extra/trunk/clangd/FindSymbols.cpp Thu Sep 13 07:27:03 2018 @@ -117,8 +117,9 @@ getWorkspaceSymbols(StringRef Query, int if (IsGlobalQuery || !Names.first.empty()) Req.Scopes = {Names.first}; if (Limit) - Req.MaxCandidateCount = Limit; - TopN<ScoredSymbolInfo, ScoredSymbolGreater> Top(Req.MaxCandidateCount); + Req.Limit = Limit; + TopN<ScoredSymbolInfo, ScoredSymbolGreater> Top( + Req.Limit ? *Req.Limit : std::numeric_limits<size_t>::max()); FuzzyMatcher Filter(Req.Query); Index->fuzzyFind(Req, [HintPath, &Top, &Filter](const Symbol &Sym) { // Prefer the definition over e.g. a function declaration in a header Modified: clang-tools-extra/trunk/clangd/index/Index.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Index.cpp Thu Sep 13 07:27:03 2018 @@ -177,14 +177,14 @@ std::shared_ptr<SymbolIndex> SwapIndex:: bool fromJSON(const llvm::json::Value &Parameters, FuzzyFindRequest &Request) { json::ObjectMapper O(Parameters); - int64_t MaxCandidateCount; + int64_t Limit; bool OK = O && O.map("Query", Request.Query) && O.map("Scopes", Request.Scopes) && - O.map("MaxCandidateCount", MaxCandidateCount) && + O.map("Limit", Limit) && O.map("RestrictForCodeCompletion", Request.RestrictForCodeCompletion) && O.map("ProximityPaths", Request.ProximityPaths); - if (OK && MaxCandidateCount <= std::numeric_limits<uint32_t>::max()) - Request.MaxCandidateCount = MaxCandidateCount; + if (OK && Limit <= std::numeric_limits<uint32_t>::max()) + Request.Limit = Limit; return OK; } @@ -192,7 +192,7 @@ llvm::json::Value toJSON(const FuzzyFind return json::Object{ {"Query", Request.Query}, {"Scopes", json::Array{Request.Scopes}}, - {"MaxCandidateCount", Request.MaxCandidateCount}, + {"Limit", Request.Limit}, {"RestrictForCodeCompletion", Request.RestrictForCodeCompletion}, {"ProximityPaths", json::Array{Request.ProximityPaths}}, }; Modified: clang-tools-extra/trunk/clangd/index/Index.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.h (original) +++ clang-tools-extra/trunk/clangd/index/Index.h Thu Sep 13 07:27:03 2018 @@ -437,9 +437,7 @@ struct FuzzyFindRequest { std::vector<std::string> Scopes; /// \brief The number of top candidates to return. The index may choose to /// return more than this, e.g. if it doesn't know which candidates are best. - // FIXME: Use llvm::Optional; semantically, the absence of MaxCandidateCount - // is equivalent to setting this field to default value as below. - uint32_t MaxCandidateCount = std::numeric_limits<uint32_t>::max(); + llvm::Optional<uint32_t> Limit; /// If set to true, only symbols for completion support will be considered. bool RestrictForCodeCompletion = false; /// Contextually relevant files (e.g. the file we're code-completing in). @@ -447,9 +445,9 @@ struct FuzzyFindRequest { std::vector<std::string> ProximityPaths; bool operator==(const FuzzyFindRequest &Req) const { - return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion, + return std::tie(Query, Scopes, Limit, RestrictForCodeCompletion, ProximityPaths) == - std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount, + std::tie(Req.Query, Req.Scopes, Req.Limit, Req.RestrictForCodeCompletion, Req.ProximityPaths); } bool operator!=(const FuzzyFindRequest &Req) const { return !(*this == Req); } @@ -476,7 +474,7 @@ public: /// each matched symbol before returning. /// If returned Symbols are used outside Callback, they must be deep-copied! /// - /// Returns true if there may be more results (limited by MaxCandidateCount). + /// Returns true if there may be more results (limited by Req.Limit). virtual bool fuzzyFind(const FuzzyFindRequest &Req, llvm::function_ref<void(const Symbol &)> Callback) const = 0; Modified: clang-tools-extra/trunk/clangd/index/MemIndex.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/MemIndex.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/MemIndex.cpp (original) +++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp Thu Sep 13 07:27:03 2018 @@ -29,7 +29,8 @@ bool MemIndex::fuzzyFind( assert(!StringRef(Req.Query).contains("::") && "There must be no :: in query."); - TopN<std::pair<float, const Symbol *>> Top(Req.MaxCandidateCount); + TopN<std::pair<float, const Symbol *>> Top( + Req.Limit ? *Req.Limit : std::numeric_limits<size_t>::max()); FuzzyMatcher Filter(Req.Query); bool More = false; for (const auto Pair : Index) { Modified: clang-tools-extra/trunk/clangd/index/dex/Dex.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Dex.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/dex/Dex.cpp (original) +++ clang-tools-extra/trunk/clangd/index/dex/Dex.cpp Thu Sep 13 07:27:03 2018 @@ -179,8 +179,8 @@ bool Dex::fuzzyFind(const FuzzyFindReque // FIXME(kbobyrev): Pre-scoring retrieval threshold should be adjusted as // using 100x of the requested number might not be good in practice, e.g. // when the requested number of items is small. - const size_t ItemsToRetrieve = 100 * Req.MaxCandidateCount; - auto Root = createLimit(move(QueryIterator), ItemsToRetrieve); + auto Root = Req.Limit ? createLimit(move(QueryIterator), *Req.Limit * 100) + : move(QueryIterator); using IDAndScore = std::pair<DocID, float>; std::vector<IDAndScore> IDAndScores = consume(*Root); @@ -188,7 +188,8 @@ bool Dex::fuzzyFind(const FuzzyFindReque auto Compare = [](const IDAndScore &LHS, const IDAndScore &RHS) { return LHS.second > RHS.second; }; - TopN<IDAndScore, decltype(Compare)> Top(Req.MaxCandidateCount, Compare); + TopN<IDAndScore, decltype(Compare)> Top( + Req.Limit ? *Req.Limit : std::numeric_limits<size_t>::max(), Compare); for (const auto &IDAndScore : IDAndScores) { const DocID SymbolDocID = IDAndScore.first; const auto *Sym = Symbols[SymbolDocID]; @@ -205,7 +206,7 @@ bool Dex::fuzzyFind(const FuzzyFindReque More = true; } - // Apply callback to the top Req.MaxCandidateCount items in the descending + // Apply callback to the top Req.Limit items in the descending // order of cumulative score. for (const auto &Item : std::move(Top).items()) Callback(*Symbols[Item.first]); Modified: clang-tools-extra/trunk/test/clangd/Inputs/requests.json URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/Inputs/requests.json?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/Inputs/requests.json (original) +++ clang-tools-extra/trunk/test/clangd/Inputs/requests.json Thu Sep 13 07:27:03 2018 @@ -1,7 +1,7 @@ -[{"MaxCandidateCount":100,"ProximityPaths":["/usr/home/user/clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp"],"Query":"OMP","RestrictForCodeCompletion":true,"Scopes":["clang::"]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"s","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sy","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"Dex","RestrictForCodeCompletion":true,"Scopes":["clang::clangd::", "clang::", "clang::clangd::dex::"]}, -{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"Variable","RestrictForCodeCompletion":true,"Scopes":[""]}] +[{"Limit":100,"ProximityPaths":["/usr/home/user/clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp"],"Query":"OMP","RestrictForCodeCompletion":true,"Scopes":["clang::"]}, +{"Limit":100,"ProximityPaths":[],"Query":"s","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, +{"Limit":100,"ProximityPaths":[],"Query":"sy","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, +{"Limit":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, +{"Limit":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]}, +{"Limit":100,"ProximityPaths":[],"Query":"Dex","RestrictForCodeCompletion":true,"Scopes":["clang::clangd::", "clang::", "clang::clangd::dex::"]}, +{"Limit":100,"ProximityPaths":[],"Query":"Variable","RestrictForCodeCompletion":true,"Scopes":[""]}] Modified: clang-tools-extra/trunk/unittests/clangd/DexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/DexTests.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/DexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/DexTests.cpp Thu Sep 13 07:27:03 2018 @@ -482,7 +482,7 @@ TEST(DexTest, FuzzyMatchQ) { URISchemes); FuzzyFindRequest Req; Req.Query = "lol"; - Req.MaxCandidateCount = 2; + Req.Limit = 2; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } @@ -498,6 +498,7 @@ TEST(DexTest, DexDeduplicate) { FuzzyFindRequest Req; Req.Query = "2"; Dex I(Symbols, URISchemes); + EXPECT_FALSE(Req.Limit); EXPECT_THAT(match(I, Req), ElementsAre("2", "2")); } @@ -505,10 +506,11 @@ TEST(DexTest, DexLimitedNumMatches) { auto I = Dex::build(generateNumSymbols(0, 100), URISchemes); FuzzyFindRequest Req; Req.Query = "5"; - Req.MaxCandidateCount = 3; + Req.Limit = 3; bool Incomplete; auto Matches = match(*I, Req, &Incomplete); - EXPECT_EQ(Matches.size(), Req.MaxCandidateCount); + EXPECT_TRUE(Req.Limit); + EXPECT_EQ(Matches.size(), *Req.Limit); EXPECT_TRUE(Incomplete); } @@ -518,7 +520,7 @@ TEST(DexTest, FuzzyMatch) { URISchemes); FuzzyFindRequest Req; Req.Query = "lol"; - Req.MaxCandidateCount = 2; + Req.Limit = 2; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } @@ -596,7 +598,7 @@ TEST(DexTest, ProximityPathsBoosting) { FuzzyFindRequest Req; Req.Query = "abc"; // The best candidate can change depending on the proximity paths. - Req.MaxCandidateCount = 1; + Req.Limit = 1; // FuzzyFind request comes from the file which is far from the root: expect // CloseSymbol to come out. Modified: clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp?rev=342138&r1=342137&r2=342138&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp Thu Sep 13 07:27:03 2018 @@ -78,10 +78,11 @@ TEST(MemIndexTest, MemIndexLimitedNumMat auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab()); FuzzyFindRequest Req; Req.Query = "5"; - Req.MaxCandidateCount = 3; + Req.Limit = 3; bool Incomplete; auto Matches = match(*I, Req, &Incomplete); - EXPECT_EQ(Matches.size(), Req.MaxCandidateCount); + EXPECT_TRUE(Req.Limit); + EXPECT_EQ(Matches.size(), *Req.Limit); EXPECT_TRUE(Incomplete); } @@ -91,7 +92,7 @@ TEST(MemIndexTest, FuzzyMatch) { RefSlab()); FuzzyFindRequest Req; Req.Query = "lol"; - Req.MaxCandidateCount = 2; + Req.Limit = 2; EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits