diff mbox series

[3/5] parse-options.c: use optbug() instead of BUG() "opts" check

Message ID patch-3.5-5a3e7609854-20220521T170939Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series usage API: add and use a bug() + BUG_if_bug() | expand

Commit Message

Ævar Arnfjörð Bjarmason May 21, 2022, 5:14 p.m. UTC
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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Junio C Hamano May 25, 2022, 9:05 p.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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 | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 7fff588a45f..5875936898f 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -474,20 +474,20 @@ static void parse_options_check(const struct option *opts)
>  			break;
>  		case OPTION_CALLBACK:
>  			if (!opts->callback && !opts->ll_callback)
> -				BUG("OPTION_CALLBACK needs one callback");
> +				optbug(opts, "OPTION_CALLBACK needs one callback");
>  			if (opts->callback && opts->ll_callback)
> -				BUG("OPTION_CALLBACK can't have two callbacks");
> +				optbug(opts, "OPTION_CALLBACK can't have two callbacks");
>  			break;
>  		case OPTION_LOWLEVEL_CALLBACK:
>  			if (!opts->ll_callback)
> -				BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
> +				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
>  			if (opts->callback)
> -				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> +				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
>  			break;

I wonder if we want to somehow warn developers against careless
conversion in the documentation.

For example, when rewriting BUG()s in the original

	if (!ptr)
		BUG("unexpected NULL in ptr");
	if (!strcmp(ptr, "(null)"))
		BUG("unexpected (null) in ptr");

into bug() followed by BUG_if_bug(), a mechanical rewrite

	if (!ptr)
		bug("unexpected NULL in ptr");
	if (!strcmp(ptr, "(null)"))
		bug("unexpected (null) in ptr");
	BUG_if_bug();

may not be correctly work, if evaluation of the latter condition is
not possible when an earlier condition holds true.  The code
structure (e.g. "if/if" -> "if/else if") might have to change in
some cases.  The conditions may have to be changed in other cases.


I think the changes made this in patch are good transformations to
use the bug()/BUG_if_bug(), of course.

>  		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.");
>  		default:
>  			; /* ok. (usually accepts an argument) */
>  		}
diff mbox series

Patch

diff --git a/parse-options.c b/parse-options.c
index 7fff588a45f..5875936898f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -474,20 +474,20 @@  static void parse_options_check(const struct option *opts)
 			break;
 		case OPTION_CALLBACK:
 			if (!opts->callback && !opts->ll_callback)
-				BUG("OPTION_CALLBACK needs one callback");
+				optbug(opts, "OPTION_CALLBACK needs one callback");
 			if (opts->callback && opts->ll_callback)
-				BUG("OPTION_CALLBACK can't have two callbacks");
+				optbug(opts, "OPTION_CALLBACK can't have two callbacks");
 			break;
 		case OPTION_LOWLEVEL_CALLBACK:
 			if (!opts->ll_callback)
-				BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
+				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
 			if (opts->callback)
-				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
 			break;
 		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.");
 		default:
 			; /* ok. (usually accepts an argument) */
 		}