On Wed, Apr 16, 2025 at 12:19:31PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > In the preceding commits we have introduced integer precisions. The > > precision merely tracks bounds of the underlying data types so that we > > don't try to for example write a `size_t` into an `unsigned`, which > > could otherwise cause out-of-bounds writes. > > > > Some options may have bounds that are stricter than the underlying data > > type. Right now, users of any such options would have to manually verify > > that the value passed to such an option is inside the expected bounds. > > This is rather tedious, and it leads to code duplication across sites > > that wish to perform such bounds checks. > > > > Introduce `OPT_*_BOUNDED()` options that alleviate this issue. Users > > can optionally specify both a lower and upper bound, and if set we will > > verify that the value passed by the user is in that range. > > > > Signed-off-by: Patrick Steinhardt <ps@xxxxxx> > > --- > > parse-options.c | 40 ++++++++++++++++++++++++++++----- > > parse-options.h | 52 +++++++++++++++++++++++++++++++++++++++++++ > > t/helper/test-parse-options.c | 5 +++++ > > t/t0040-parse-options.sh | 33 +++++++++++++++++++++++++++ > > 4 files changed, 125 insertions(+), 5 deletions(-) > > It is certainly cute, but unless there are plenty of existing users > that use OPT_INTEGER() and friends and perform bounds checks > themselves, I am not sure if this can withstand YAGNI criticism. > And this step being at the end of the series, plus the above > diffstat, tells us that there aren't any existing users converted to > use this new mechanism. Yeah, that was also a bit of my feeling. I was on the lookout for callsites, but I ultimately didn't find too many. Which is basically the reason why I said that this patch is more of a PoC, and that I'm happy to drop it again. > OPT_INTEGER that wants to track percentage may want to say the value > is between 0 and 100 (inclusive), but instead we take it bounded not > to exceed 100, without lower bound. Without a real callsite, we > cannot even tell if it is acceptable compromise for the sake of > simplicity to forbid 0 as lower or upper bound, for example. Yes, `0` meaning "default" is restricting us here. But my counter argument is that a value that can only be between `0` and `100` should use `OPT_UNSIGNED` in the first place, which allows us to achieve exactly that. Let's just drop this patch for now. It was only a PoC anyway, and we can use it as inpiration if we ever see that this feature is something we want. Patrick
