sammccall added inline comments.
================
Comment at: clangd/XRefs.cpp:724
}
-
+ if (!Limit)
+ Limit = std::numeric_limits<uint32_t>::max();
----------------
nit: move this to the top, since it's just adjusting input params?
================
Comment at: clangd/XRefs.cpp:730
for (const auto &Ref : MainFileRefs) {
+ if (Results.size() == Limit)
+ break;
----------------
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)
```
================
Comment at: clangd/index/MemIndex.cpp:72
trace::Span Tracer("MemIndex refs");
+ uint32_t Limit =
+ Req.Limit ? *Req.Limit : std::numeric_limits<uint32_t>::max();
----------------
Req.Limit.getValueOr(...)
================
Comment at: clangd/index/MemIndex.cpp:72
trace::Span Tracer("MemIndex refs");
+ uint32_t Limit =
+ Req.Limit ? *Req.Limit : std::numeric_limits<uint32_t>::max();
----------------
sammccall wrote:
> Req.Limit.getValueOr(...)
if you're going to mutate this, maybe call it "remaining" or so?
================
Comment at: clangd/index/Merge.cpp:98
// Ultimately we should explicit check which index has the file instead.
+ uint32_t DynamicCount = 0;
+ uint32_t Limit =
----------------
why two separate count variables?
================
Comment at: clangd/index/Merge.cpp:98
// Ultimately we should explicit check which index has the file instead.
+ uint32_t DynamicCount = 0;
+ uint32_t Limit =
----------------
sammccall wrote:
> why two separate count variables?
you've inserted the new code between existing code and the comment that
describes it.
================
Comment at: unittests/clangd/DexTests.cpp:673
- std::vector<std::string> Files;
- RefsRequest Req;
- Req.IDs.insert(Foo.ID);
- Req.Filter = RefKind::Declaration | RefKind::Definition;
- Dex(std::vector<Symbol>{Foo, Bar}, Refs).refs(Req, [&](const Ref &R) {
- Files.push_back(R.Location.FileURI);
- });
-
- EXPECT_THAT(Files, ElementsAre("foo.h"));
+ {
+ std::vector<std::string> Files;
----------------
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.
================
Comment at: unittests/clangd/DexTests.cpp:686
+ Req.IDs.insert(Foo.ID);
+ Req.Limit = 1;
+ size_t RefCount = 0;
----------------
As far as I can tell, this limit makes no difference, there's only one result
anyway.
================
Comment at: unittests/clangd/DexTests.cpp:691
+ });
+ EXPECT_THAT(*Req.Limit, RefCount);
+ }
----------------
In tests, we know what the number is, and it's clearer to just spell it out:
`EXPECT_EQ(1, RefCount)`
================
Comment at: unittests/clangd/DexTests.cpp:691
+ });
+ EXPECT_THAT(*Req.Limit, RefCount);
+ }
----------------
sammccall wrote:
> In tests, we know what the number is, and it's clearer to just spell it out:
> `EXPECT_EQ(1, RefCount)`
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"))
================
Comment at: unittests/clangd/IndexTests.cpp:306
+ {
+ Request.Limit = 1;
+ size_t RefsCount = 0;
----------------
why new scope here?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56597/new/
https://reviews.llvm.org/D56597
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
