Re: [PATCH 2/5] parse-options: introduce precision handling for `OPTION_INTEGER`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 15, 2025, 10:26 UTC
- Message-ID
- <Z_40Zv_0HCscTo0M@pks.im>
- In-Reply-To
- <f6c7c92b-931c-4be4-8d76-9d4ed147e983@web.de>
On Tue, Apr 01, 2025 at 08:47:12PM +0200, René Scharfe wrote:
Show 14 quoted lines
> Am 01.04.25 um 17:01 schrieb Patrick Steinhardt:
> > diff --git a/parse-options.c b/parse-options.c
> > index 35fbb3b0d63..dbda9b7cfe7 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -172,25 +172,50 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
> > return (*opt->ll_callback)(p, opt, p_arg, p_unset);
> > }
> > case OPTION_INTEGER:
> > + {
> > + intmax_t upper_bound = (((intmax_t) 1 << (opt->precision * 8 - 1)) - 1);
>
> Ugh, how does this not overflow? The macro maximum_signed_value_of_type
> does a similar calculation better.Oh, nice :) I'll definitely use this, thanks!
> > + intmax_t lower_bound = -upper_bound - 1; > > This depends on two's complement being used, which is bad for purity and > portability to obsolete machines, but probably OK in practice.
Agreed. We would notice quite fast in case there are any machines out there that don't handle this well as our tests exercise this logic.
Show 52 quoted lines
> > + intmax_t value;
> > +
> > if (unset) {
> > - *(int *)opt->value = 0;
> > - return 0;
> > - }
> > - if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
> > - *(int *)opt->value = opt->defval;
> > - return 0;
> > - }
> > - if (get_arg(p, opt, flags, &arg))
> > + value = 0;
> > + } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
> > + value = opt->defval;
> > + } else if (get_arg(p, opt, flags, &arg)) {
> > return -1;
> > - if (!*arg)
> > + } else if (!*arg) {
> > return error(_("%s expects a numerical value"),
> > optname(opt, flags));
> > - *(int *)opt->value = strtol(arg, (char **)&s, 10);
> > - if (*s)
> > - return error(_("%s expects a numerical value"),
> > - optname(opt, flags));
> > - return 0;
> > + } else {
> > + value = strtoimax(arg, (char **)&s, 10);
> > + if (*s)
> > + return error(_("%s expects a numerical value"),
> > + optname(opt, flags));
> > +
> > + }
> >
> > + if (value < lower_bound || value > upper_bound)
> > + return error(_("value %"PRIdMAX" for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
> > + value, optname(opt, flags), lower_bound, upper_bound);
> > +
> > + switch (opt->precision) {
> > + case 1:
> > + *(int8_t *)opt->value = value;
> > + return 0;
> > + case 2:
> > + *(int16_t *)opt->value = value;
> > + return 0;
> > + case 4:
> > + *(int32_t *)opt->value = value;
> > + return 0;
> > + case 8:
> > + *(int64_t *)opt->value = value;
> > + return 0;
>
> Do we even need all these sizes? Or can we whittle it down to ssize_t?Some of them. We already pass both `int` and `size_t`, which means that we definitely need at least `int32` and `int64`. Whether we also need to handle `int8` or `int16` is a different question, but it doesn't add much complexity anyway.
> And for which quantities do we need to accept negative values anyway?
In most cases we probably don't need that, agreed. But there are callsites where we use negative values to indicate the default value, and I bet there are some options where negative values do make sense indeed.
But in the end I think we should harden our integer handling for options, which is where the next patch comes in that introduces `OPTION_UNSIGNED()`. I don't want to convert all users in this patch series, but I definitely think that we should adapt them over time so that they stop accepting signed integers.
Patrick