[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

[Sam McCall via Phabricator via cfe-commits]

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