Page MenuHomePhabricator

[clangd] NFC: Use uint32_t for FuzzyFindRequest limits
ClosedPublic

Authored by kbobyrev on Sep 10 2018, 8:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kbobyrev created this revision.Sep 10 2018, 8:43 AM
ioeric added inline comments.Sep 10 2018, 8:56 AM
clang-tools-extra/clangd/index/Index.cpp
186 ↗(On Diff #164680)

I think we should not set Request.MaxCandidateCount if MaxCandidateCount is greater than std::numeric_limits<int32_t>::max()?

clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

Or use unsigned?

Why this change?

(If you're changing it, Optional<unsigned> is probably clearer)

kbobyrev updated this revision to Diff 164821.Sep 11 2018, 1:05 AM
kbobyrev marked an inline comment as done.
kbobyrev added inline comments.
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

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?

ioeric accepted this revision.Sep 11 2018, 1:30 AM
ioeric added inline comments.
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

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.

This revision is now accepted and ready to land.Sep 11 2018, 1:30 AM
ilya-biryukov added inline comments.Sep 11 2018, 2:19 AM
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

BTW, many people think using unsigned ints just because inputs can't be negative is a bad idea.
See https://stackoverflow.com/a/18796234

sammccall added inline comments.Sep 11 2018, 2:26 AM
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

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)

ioeric added inline comments.Sep 11 2018, 2:41 AM
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

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.

ilya-biryukov added inline comments.Sep 11 2018, 2:50 AM
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

+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.

kbobyrev updated this revision to Diff 164836.Sep 11 2018, 3:30 AM
kbobyrev added inline comments.
clang-tools-extra/clangd/index/Index.h
440 ↗(On Diff #164680)

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.

This revision was automatically updated to reflect the committed changes.