Page MenuHomePhabricator

[clangd] Support filtering by fixing scopes in fuzzyFind.
ClosedPublic

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.

clangd/index/Index.h
130

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.

135

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.

clangd/index/MemIndex.cpp
31–33

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?

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!

clangd/index/Index.h
130

This makes a lot of sense.

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

just nits, all this stuff up to you

clangd/index/Index.h
131

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

clangd/index/MemIndex.cpp
38

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

clangd/index/SymbolCollector.cpp
63

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

unittests/clangd/FileIndexTests.cpp
127

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.