On Sun, Feb 18, 2018 at 2:35 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> 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.
Referring to this version that is:
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);
}
}
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
