[PATCH] D56597: [clangd] Add Limit parameter for xref.

[Sam McCall via Phabricator via cfe-commits]

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