On Sun, Feb 18, 2018 at 2:19 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> 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
One last bikeshedding: Could factor out just the check into a function
and do something like this in __expr_print():
if (select_has_type(e, type))
expr_print_newline(e, fn, data, prevtoken);
Having a function that's just one big 'if' feels slightly silly,
though it's not horrible here either IMO.
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
