Re: [PATCH v3 7/7] parse-options: introduce bounded integer options

[Patrick Steinhardt <ps@pks.im>]

On Wed, Apr 16, 2025 at 12:19:31PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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@pks.im>
> > ---
> >  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