Re: [PATCH 07/14] parse-options: allow ll_callback with OPTION_CALLBACK

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 16, 2019, 08:52 UTC
Message-ID: <CACsJy8DDKuK5VYhh0GNYSJK1_y3MZgK5Vcq99N4jYcusVFnvQQ@mail.gmail.com>
In-Reply-To: <02d2a828-b191-9d9a-7422-d76cdca69ef1@gmail.com>

On Mon, Apr 15, 2019 at 9:06 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 20 quoted lines

>
> On 1/26/2019 7:35 PM, Nguyễn Thái Ngọc Duy wrote:
> > @@ -238,7 +249,10 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
> >                       len++;
> >               arg = xmemdupz(p->opt, len);
> >               p->opt = p->opt[len] ? p->opt + len : NULL;
> > -             rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             if (numopt->callback)
> > +                     rc = (*numopt->callback)(numopt, arg, 0) ? (-1) : 0;
> > +             else
> > +                     rc = (*numopt->ll_callback)(p, numopt, arg, 0);
> >               free(arg);
> >               return rc;
> >       }
>
> Hi Duy,
>
> This "else" condition is unreachable. This block is only hit when we have a "-<n>"
> option, using OPT_NUMBER_CALLBACK, which is implemented by filling "callback", never
> "ll_callback".

It does not mean ll_callback cannot be used in the future though. We have three options

1. drop the else clause
2. replace with "else BUG();"
3. implement proper else clause

Option #1 to me sounds wrong. If you don't support something, yell up. Silently ignoring it only makes it harder to track down to this unsupported location when it becomes reachable, however unlikely that is.

Which leaves options #2 and #3. If you think this one line is risky enough, I'll send a patch to replace it with BUG().

> I recommend reverting this diff segment, but please let me know if I'm missing something.
>
> Thanks,
> -Stolee

-- 
Duy