sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:74 auto Names = splitQualifiedName(Query); ---------------- Add a comment here (or elsewhere, I guess) about how qualified names are handled. - exact namespace: boosted on the index side - approximate namespace (e.g. substring): included using "all scopes" logic - non-matching namespace (no subtring match): filtered out from index-provided results ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:83 + // Boost symbols from desired namespace. + if (!Req.AnyScope || !Names.first.empty()) Req.Scopes = {std::string(Names.first)}; ---------------- This expression doesn't make sense (except in context of the above code). Variables are cheap! ``` LeadingColons = consume_front("::"); Req.AnyScope = !LeadingColons if (LeadingColons || !Names.first.empty()) ... ``` ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:86 if (Limit) Req.Limit = Limit; TopN<ScoredSymbolInfo, ScoredSymbolGreater> Top( ---------------- Given the dynamic filter, we should request a greater multiple here (this time if anyscope && Names.first.empty is the right logic!) This gives us a second class of regression :-) but we can tune the multiple to control it. I'd suggest 5 or so ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:93 + llvm::StringRef ScopeRef = Scope; + // Fuzzfind might return symbols from irrelevant namespaces if query was not + // fully-qualified, drop those. ---------------- nit: fuzzyfind ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:95 + // fully-qualified, drop those. + if (!ScopeRef.contains(Names.first)) + return; ---------------- Pull out a `approximateScopeMatch(scope, query)` or so function? Substring isn't quite right - fine for now with a fixme, but might as well pull out the function now so we don't make a mess when the code grows. ================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:111 Quality.merge(Sym); SymbolRelevanceSignals Relevance; Relevance.Name = Sym.Name; ---------------- Would be nice to incorporate exact vs approximate scope match here: people complain a lot when an exact string match ranks below an approximate one. I don't think ScopeDistance works as-is, because it assumes the reference point (query) is absolute. You could consider NeedsFixIts or applying a multiplier to NameMatch (0.3?) or InBaseClass (all of these are a stretch I guess) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88814/new/ https://reviews.llvm.org/D88814 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits