[PATCH] D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Index.h:440
   /// return more than this, e.g. if it doesn't know which candidates are best.
-  size_t MaxCandidateCount = std::numeric_limits<size_t>::max();
+  uint32_t MaxCandidateCount = std::numeric_limits<uint32_t>::max();
   /// If set to true, only symbols for completion support will be considered.
----------------
ioeric wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > kbobyrev wrote:
> > > > > ioeric wrote:
> > > > > > Or use `unsigned`?
> > > > > `unsigned` would have different size on different platforms, I'm not really sure we want that; could you elaborate on why you think that would be better?
> > > > I thought it's (almost) always 4 bytes? But it should always have a smaller size than `uint64_t` in json serialization, so it should work for us. In general, I would prefer `unsigned` to `uint32_t` when possible. For most of the platforms, they are the same. But up to you :) I don't really feel strong about this.
> > > BTW, many people think using unsigned ints just because inputs can't be negative is a bad idea.
> > > See https://stackoverflow.com/a/18796234
> > I mostly agree with that, but LLVM uses unsigned types pervasively, and -Wsign-compare, so they're hard to avoid.
> > 
> > (FWIW, I still think that this case has become complicated enough that we should use the most explicit option, which seems like Optional here)
> The complication seems to be mostly in json serialization of the field, which is more of an implementation detail I think. If we could get away with using max unsigned as no limit, which seems to have worked well so far, I would still prefer it over `Optional`, as it is less confusing on the reference site and involves less refactoring.
+1 to using Optional, this states the contract better and actually gives a clear model of how to should be serialized and deserialized.

Can see the logic in using large values for the limits too, but optional seems to avoid various corner cases, which outweighs the refactoring costs IMO.

Feel free to ignore, though, just a drive-by-comment.


https://reviews.llvm.org/D51860