Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups

[Ulf Magnusson <ulfalizer@gmail.com>]

On Sun, Feb 18, 2018 at 12:07:02PM +0100, Eugeniu Rosca wrote:
> If expr_print_newline is renamed to expr_print_{select,revdep}, then
> I still face the need of expr_print_newline. So, I kept the *newline()
> function in place and came up with below solution to aggregate
> duplicated code. Please, let me know if it looks OK to you (btw, it
> creates a slightly higher --stat compared to current solution).
> 
> diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> index 48b99371d276..a6316e5681d9 100644
> --- a/scripts/kconfig/expr.c
> +++ b/scripts/kconfig/expr.c
> @@ -1189,6 +1189,34 @@ expr_print_newline(struct expr *e,
>  	expr_print(e, fn, data, prevtoken);
>  }
>  
> +static void
> +expr_print_revdep(struct expr *e,
> +		  void (*fn)(void *, struct symbol *, const char *),
> +		  void *data,
> +		  int prevtoken,
> +		  enum print_type type)
> +{
> +	switch (type) {
> +	case PRINT_REVDEP_ALL:
> +		expr_print_newline(e, fn, data, prevtoken);
> +		break;
> +	case PRINT_REVDEP_YES:
> +		if (expr_calc_value(e) == yes)
> +			expr_print_newline(e, fn, data, prevtoken);
> +		break;
> +	case PRINT_REVDEP_MOD:
> +		if (expr_calc_value(e) == mod)
> +			expr_print_newline(e, fn, data, prevtoken);
> +		break;
> +	case PRINT_REVDEP_NO:
> +		if (expr_calc_value(e) == no)
> +			expr_print_newline(e, fn, data, prevtoken);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +

I think it's fine to have a separate expr_print_newline() function
still. Getting rid of the repetition still makes it more readable to me.

Maybe you could do something like this if you want to eliminate
expr_print_newline() though. AFAICS, it isn't used outside
expr_print_revdep().

	static void
	expr_print_revdep(struct expr *e,
			  void (*fn)(void *, struct symbol *, const char *),
			  void *data,
			  int prevtoken,
			  enum print_type type)
	{
		if (type == PRINT_REVDEP_ALL                              ||
		    type == PRINT_REVDEP_YES && expr_calc_value(e) == yes ||
		    type == PRINT_REVDEP_MOD && expr_calc_value(e) == mod ||
		    type == PRINT_REVDEP_NO  && expr_calc_value(e) == no) {

			fn(data, NULL, "\n - ");
			expr_print(e, fn, data, prevtoken);
		}
	}


Could turn that into a switch over 'type' and set a 'print_revdep' flag
which is checked at the end too, if you'd prefer that:

	switch (type) {
	...
	case PRINT_REVDEP_YES:
		print_revdep = (expr_calc_value(e) == yes);
		break;
	...
	}

	if (print_revdep) {
		fn(data, NULL, "\n - ");
		expr_print(e, fn, data, prevtoken);
	}


Or return early:

	case PRINT_REVDEP_YES:
		if (expr_calc_value(e) != yes)
			return;
		break;
	...
	}

	fn(data, NULL, "\n - ");
	expr_print(e, fn, data, prevtoken);


The original version is already fine by me though. The 'if' version
seems pretty direct too.

Cheers,
Ulf