On Sun, Feb 18, 2018 at 2:02 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> 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
Hmm... was thinking that you could get rid of 'print_type' and just
pass a tristate value for a moment too. There's PRINT_NORMAL and
PRINT_REVDEP_ALL to deal with as well though, so it'd get messy.
Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
