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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
