On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> Change the assertions added in bf3ff338a25 (parse-options: stop
> abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
> instead of BUG(). At this point we're looping over individual options,
> so if we encounter any issues we'd like to report the offending option.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> parse-options.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 78b46ae9698..243016ae30f 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -473,21 +473,30 @@ static void parse_options_check(const struct option *opts)
> optbug(opts, "should not accept an argument");
> break;
> case OPTION_CALLBACK:
> - if (!opts->callback && !opts->ll_callback)
> - BUG("OPTION_CALLBACK needs one callback");
> - if (opts->callback && opts->ll_callback)
> - BUG("OPTION_CALLBACK can't have two callbacks");
> + if (!opts->callback && !opts->ll_callback) {
> + optbug(opts, "OPTION_CALLBACK needs one callback");
> + break;
> + }
> + if (opts->callback && opts->ll_callback) {
> + optbug(opts, "OPTION_CALLBACK can't have two callbacks");
> + break;
> + }
> break;
> case OPTION_LOWLEVEL_CALLBACK:
> - if (!opts->ll_callback)
> - BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
> - if (opts->callback)
> - BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> + if (!opts->ll_callback) {
> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
> + break;
> + }
> + if (opts->callback) {
> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> + break;
> + }
> break;
A minor point, but I'm not sure I understand why we're adding breaks for
the two cases above. In the OPTION_CALLBACK case, the if conditions are
mutually exclusive and are followed by an unconditional break, so adding
additional breaks seems unnecessary. For the OPTION_LOWLEVEL_CALLBACK
case, the conditions are not mutually exclusive, but isn't this exactly
the issue that optbug() is intended to address? I.e., wouldn't the
caller want to see both optbug()s if both are relevant?
> case OPTION_ALIAS:
> - BUG("OPT_ALIAS() should not remain at this point. "
> - "Are you using parse_options_step() directly?\n"
> - "That case is not supported yet.");
> + optbug(opts, "OPT_ALIAS() should not remain at this point. "
> + "Are you using parse_options_step() directly?\n"
> + "That case is not supported yet.");
> + break;
> default:
> ; /* ok. (usually accepts an argument) */
> }
> --
> 2.36.1.1100.g16130010d07
>
