ioeric added a comment. Thanks for the suggestions! After taking a closer look at boosting factors for other signals, I think I was being too conservative about boosting preferred scopes and penalizing non-preferred ones. I have tuned the parameters to make these more aggressive now according to your suggestion.
================ Comment at: clangd/Quality.cpp:262 + Opts.UpCost = 1; + Opts.DownCost = 1; + Opts.AllowDownTraversalFromRoot = false; ---------------- sammccall wrote: > I thought we concluded down cost was likely to be significantly higher than > up cost? Sorry, forgot to make that change. Made UpCost=1, DownCost=2 ================ Comment at: clangd/Quality.cpp:272 + else if (!S.empty() && Preferred.startswith(S)) + Param.Cost = Opts.UpCost; + else ---------------- sammccall wrote: > This seems like an odd rule, what's the intuition? > - if the preferred scope is a::b::c:: then we're considering a::b:: and a:: > to be equal, rather than preferring the former > - you're defining the cost in terms of UpCost, but it's not clear why - is > this just being used as a scale? > - for the direct parent of the preferred scope, this is a no-op. So this > only does anything when the preferred scope is at least 3 deep > > As a first approximation, `; // just rely on up-traversals` might be OK, > otherwise, I guess you're trying to boost these compared to just relying on > up-traversals, so I'd expect this to look something like `Cost = UpCost * > (len(preferred) - len(S)) / 2` > As a first approximation, ; // just rely on up-traversals might be OK, This sounds good to me. ================ Comment at: clangd/Quality.cpp:276 + if (S == "") + Param.Cost += Opts.UpCost; + Sources[Path.first] = std::move(Param); ---------------- sammccall wrote: > Why is this +=? > > Seems like there's three cases here: > - global is the preferred scope: I think the cost has to be 0 in this case, > no? > - global is another acceptable scope: some large fixed cost seems appropriate > - global is not listed: no source to worry about > global is the preferred scope: I think the cost has to be 0 in this case, no? I think we should still apply some penalty here because 1. All projects can define symbols in the global scope. For example, if I work on a project without namespace, symbols from global namespaces are not necessarily relevant. 2. A use pattern is that using-namespace is used in place of enclosing namespaces in implementation files. Otherwise, done. ================ Comment at: clangd/Quality.cpp:287 + auto D = Distance->distance(scopeToPath(Scope).first); + return std::max(0.6, MaxBoost - D * 0.25); +} ---------------- sammccall wrote: > 0.6 seems too high for AnyScope matches, to me. And you're not distinguishing > anyScope matches from bad path matches. > > I think the linear scale is too flat: if you're in clang::clangd::, then > clangd symbols will only get a 13% boost over clang ones. > > Given the current UpCost == DownCost == 1, I'd consider something like > ``` > if (D == FileDistance::Unreachable) > return 0.1; > return max(0.5, MaxBoost * pow(0.6, D)) > ``` Done except for the scope for unreachable paths. I think 0.1 is too much penalty and would probably make cross-namespace completions not useful. As cross-namespace completion is not enabled by default, it should be safe to give it a higher score (0.4?). ================ Comment at: clangd/Quality.cpp:382 + // always in the accessible scope. + Score *= SemaSaysInScope ? QueryScopeProximity::MaxBoost + : scopeBoost(*ScopeProximityMatch, SymbolScope); ---------------- sammccall wrote: > hmm, if you have no scopes in the query I think you're boosting the sema > results and not the index ones. Intended? Not intended. Changed to only boost scopes if there are query scopes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53131 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
