Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
- From
Josh Steadmon <steadmon@google.com>
- Date
- May 31, 2022, 17:38 UTC
- Message-ID
- <YpZSrNM/bZ3i6Yvu@google.com>
- In-Reply-To
- <patch-v2-4.6-47d384d0ae5-20220531T164806Z-avarab@gmail.com>
On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
Show 45 quoted lines
> 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@gmail.com>
> ---
> 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?
Show 14 quoted lines
> 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
>