Page MenuHomePhabricator

[clangd] Support filtering by fixing scopes in fuzzyFind.

Authored by ioeric on Dec 18 2017, 1:46 PM.

Diff Detail

Event Timeline

ioeric created this revision.Dec 18 2017, 1:46 PM
ioeric updated this revision to Diff 127409.Dec 18 2017, 2:00 PM
  • minor cleanup.

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.


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.


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.


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.


ioeric updated this revision to Diff 127478.Dec 19 2017, 3:03 AM
ioeric marked 3 inline comments as done.
  • Address review comments.

Thanks for the review!


This makes a lot of sense.

sammccall accepted this revision.Dec 19 2017, 3:19 AM

just nits, all this stuff up to you


I think the rest of the comment isn't needed now - the first two sentences are enough (plus the comment on Scopes)


This is too early - we may not actually have an N+1th match :-(


If you'll never have leading ::, assert that.
Otherwise strip it (I think an assert is better unless you actually expect it)


nit: i usually find Req.Scopes = {"ns"} more clear, up to you

This revision is now accepted and ready to land.Dec 19 2017, 3:19 AM
ioeric updated this revision to Diff 127484.Dec 19 2017, 3:33 AM
ioeric marked 3 inline comments as done.
  • Address a few more comments.
ioeric updated this revision to Diff 127486.Dec 19 2017, 3:36 AM
  • Minor cleanup
This revision was automatically updated to reflect the committed changes.