[PATCH] D41367: [clangd] Support filtering by fixing scopes in fuzzyFind.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice, thanks!
Some suggestions for further tightening the semantics, I'm happy to do these as a followup if you think it's too invasive here.



================
Comment at: clangd/index/Index.h:127
   /// \brief A query string for the fuzzy find. This is matched against symbols'
-  /// qualfified names.
+  /// qualified names. If any scope below is provided, \p Query is only matched
+  /// against symbols in the scope (excl. symbols in nested scopes).
----------------
If i'm reading this right, you still support fuzzy-matching the scopes (scope information can be in Query, or Scopes, or both). Why? Semantics would be simpler if query matched the identifier only.


================
Comment at: clangd/index/Index.h:133
+  /// is provided, the matched symbols must be defined in scope "xyz" but not
+  /// "xyz::abc". "" and "::" are interpreted as the global namespace.
+  std::vector<std::string> Scopes;
----------------
nit: can we choose one?
i.e. we should mandate one of 
 - `{"", "foo", "foo::bar"}`
 - `{"", "::foo", "::foo::bar"}`
 - `{"", "foo::", "foo::bar::"}`
 - `{"::", "::foo::", "::foo::bar::"}`
Allowing multiple options is somewhat confusing to callers and puts a bigger burden on implementers of this interface.


================
Comment at: clangd/index/MemIndex.cpp:30
                          std::function<void(const Symbol &)> Callback) const {
-  std::string LoweredQuery = llvm::StringRef(Req.Query).lower();
+  std::vector<std::string> FixedPrefixes;
+  for (StringRef Scope : Req.Scopes)
----------------
Rather than doing tricky matches against QName, it may be simpler to split Symbol::QName into Symbol::Scope + Symbol::Name. In addition to this becoming exact string matching without parsing, the Symbol::Scopes will have lots of duplicates that can share storage, reducing the size of each Symbol.

WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41367