This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add Limit parameter for xref.
ClosedPublic

Authored by hokein on Jan 11 2019, 6:58 AM.

Event Timeline

hokein created this revision.Jan 11 2019, 6:58 AM
hokein updated this revision to Diff 181481.Jan 13 2019, 12:19 PM

Fix an issue and add comments.

sammccall added inline comments.Jan 14 2019, 3:36 AM
clangd/XRefs.cpp
726

nit: move this to the top, since it's just adjusting input params?

731

I don't think we need to break up the flow here to early exit.

I'd suggest reducing the number of places we deal with limits, we can get away with 3 instead of 5:

... collect results as before ...
if (Index && Limit && Results.size() < Limit) {
  ... query index ...
  Req.Limit = Limit;
  ... append results as usual ...
}
if (Limit && Results.size() > Limit)
  Results.resize(limit)
clangd/index/MemIndex.cpp
72

Req.Limit.getValueOr(...)

72

if you're going to mutate this, maybe call it "remaining" or so?

clangd/index/Merge.cpp
100

why two separate count variables?

100

you've inserted the new code between existing code and the comment that describes it.

unittests/clangd/DexTests.cpp
684–689

If you want these to be the same test, there should be some connection between them.
A natural one is that it's the same query with a different limit - you could extend the original test to have multiple results.

697

As far as I can tell, this limit makes no difference, there's only one result anyway.

702

In tests, we know what the number is, and it's clearer to just spell it out: EXPECT_EQ(1, RefCount)

702

generally prefer to use EXPECT_THAT only with matchers. EXPECT_EQ gives clearer error messages, reads more naturally, and is more common.

Better still would be to assert the actual outcomes, e.g. ElementsAre(anyOf("foo1.h", "foo2.h"))

unittests/clangd/IndexTests.cpp
315

why new scope here?

hokein updated this revision to Diff 181543.Jan 14 2019, 6:01 AM
hokein marked 14 inline comments as done.

Address comments.

hokein added inline comments.Jan 14 2019, 6:05 AM
unittests/clangd/DexTests.cpp
684–689

Done, made the query request the same in this test except the limit.

697

Actually, the filter of the request was different than the case above, we would get 2 refs. Used the same request to avoid confusion.

unittests/clangd/IndexTests.cpp
315

To align with the case above.

sammccall accepted this revision.Jan 14 2019, 7:47 AM
sammccall added inline comments.
unittests/clangd/DexTests.cpp
691

instead of splitting scopes here, might be more obvious just to Files.clear() between?

unittests/clangd/IndexTests.cpp
315

FWIW I'd find it easier to read if neither this nor the one above used new scopes, which tend to suggest some spooky scoped object side effects to me.
Up to you, though.

This revision is now accepted and ready to land.Jan 14 2019, 7:47 AM
hokein updated this revision to Diff 181575.Jan 14 2019, 10:01 AM
hokein marked an inline comment as done.

Remove scopes.

This revision was automatically updated to reflect the committed changes.