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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev 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.
----------------
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)
Yes, AFAIK Google Coding Guidelines prohibit usage of unsigned (e.g. in `for` loops), but I don't know whether I feel good about that.

As for the `llvm::Optional`, I can understand arguments for and against it, but it might be OK to use the proposed approach for now as it doesn't require any refactoring and add optional later if I (or someone else) has time to make sure the user code doesn't make anything unexpected. I'll put a `FIXME` on that, will probably fix later if I'll get enough time. Also, it might make sense to rename it to `Limit` or something similar.


https://reviews.llvm.org/D51860