| Message ID | 20180218110702.GA26185@example.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 18, 2018 at 2:02 PM, Ulf Magnusson <ulfalizer@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 18, 2018 at 2:19 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Sun, Feb 18, 2018 at 2:02 PM, Ulf Magnusson <ulfalizer@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 18, 2018 at 2:35 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Sun, Feb 18, 2018 at 2:19 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> On Sun, Feb 18, 2018 at 2:02 PM, Ulf Magnusson <ulfalizer@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + } +} + static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), @@ -1211,21 +1239,10 @@ __expr_print(struct expr *e, fn(data, e->left.sym, e->left.sym->name); break; case PRINT_REVDEP_ALL: - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_YES: - if (expr_calc_value(e) == yes) - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_MOD: - if (expr_calc_value(e) == mod) - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_NO: - if (expr_calc_value(e) == no) - expr_print_newline(e, fn, data, E_OR); - break; - default: + expr_print_revdep(e, fn, data, E_OR, type); break; } else @@ -1283,21 +1300,10 @@ __expr_print(struct expr *e, expr_print(e->right.expr, fn, data, E_AND); break; case PRINT_REVDEP_ALL: - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_YES: - if (expr_calc_value(e) == yes) - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_MOD: - if (expr_calc_value(e) == mod) - expr_print_newline(e, fn, data, E_OR); - break; case PRINT_REVDEP_NO: - if (expr_calc_value(e) == no) - expr_print_newline(e, fn, data, E_OR); - break; - default: + expr_print_revdep(e, fn, data, E_OR, type); break; } break; > > > default: > > break; > > } > > @@ -1273,6 +1285,18 @@ __expr_print(struct expr *e, > > case PRINT_REVDEP_ALL: > > expr_print_newline(e, fn, data, E_OR); > > break; > > + case PRINT_REVDEP_YES: > > + if (expr_calc_value(e) == yes) > > + expr_print_newline(e, fn, data, E_OR); > > + break; > > + case PRINT_REVDEP_MOD: > > + if (expr_calc_value(e) == mod) > > + expr_print_newline(e, fn, data, E_OR); > > + break; > > + case PRINT_REVDEP_NO: > > + if (expr_calc_value(e) == no) > > + expr_print_newline(e, fn, data, E_OR); > > + break; > > That would simplify this part as well, since they're equal. > > > default: > > break; > > } > > @@ -1353,10 +1377,43 @@ void expr_gstr_print(struct expr *e, struct gstr *gs) > > expr_print(e, expr_print_gstr_helper, gs, E_NONE); > > } > > > > +/* > > + * Allow front ends to check if a specific reverse dependency expression > > + * has at least one top level "||" member which evaluates to "val". This, > > + * will allow front ends to, as example, avoid printing "Selected by [y]:" > > + * line when there are actually no top level "||" sub-expressions which > > + * evaluate to =y. > > + */ > > +bool expr_revdep_contains(struct expr *e, tristate val) > > +{ > > + bool ret = false; > > + > > + if (!e) > > + return ret; > > + > > + switch (e->type) { > > + case E_SYMBOL: > > + case E_AND: > > + if (expr_calc_value(e) == val) > > + ret = true; > > + break; > > + case E_OR: > > + if (expr_revdep_contains(e->left.expr, val)) > > + ret = true; > > + else if (expr_revdep_contains(e->right.expr, val)) > > + ret = true; > > + break; > > + default: > > + break; > > + } > > + return ret; > > +} > > + > > /* > > * Transform the top level "||" tokens into newlines and prepend each > > * line with a minus. This makes expressions much easier to read. > > - * Suitable for reverse dependency expressions. > > + * Suitable for reverse dependency expressions. In addition, allow > > + * selective printing of tokens/sub-expressions by their tristate value. > > */ > > void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t) > > { > > diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h > > index 21cb67c15091..d5b096725ca8 100644 > > --- a/scripts/kconfig/expr.h > > +++ b/scripts/kconfig/expr.h > > @@ -37,6 +37,9 @@ enum expr_type { > > enum print_type { > > PRINT_NORMAL, > > PRINT_REVDEP_ALL, > > PRINT_REVDEP_ALL is unused now, right? > > Guess it doesn't hurt that much to have it there, though I'm more of a > "it won't be needed" person. Correct. It remains unused if this patchset is applied. I would avoid removing PRINT_REVDEP_ALL in the same context with adding support for PRINT_REVDEP_{YES|MOD|NO}. I think this should be done in a standalone commit for better visibility (to be reverted easily if ever needed). Here is how it would look like on top of v3 patchset. Please, provide your preference if the 6 lines may stay or should be gone. diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index b91cbc1e20c0..2313a157ebaf 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1199,9 +1199,6 @@ expr_print_revdep(struct expr *e, switch (type) { case PRINT_NORMAL: break; - 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); @@ -1238,7 +1235,6 @@ __expr_print(struct expr *e, case PRINT_NORMAL: fn(data, e->left.sym, e->left.sym->name); break; - case PRINT_REVDEP_ALL: case PRINT_REVDEP_YES: case PRINT_REVDEP_MOD: case PRINT_REVDEP_NO: @@ -1299,7 +1295,6 @@ __expr_print(struct expr *e, fn(data, NULL, " && "); expr_print(e->right.expr, fn, data, E_AND); break; - case PRINT_REVDEP_ALL: case PRINT_REVDEP_YES: case PRINT_REVDEP_MOD: case PRINT_REVDEP_NO: diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index d5b096725ca8..e5687b430c17 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -36,7 +36,6 @@ enum expr_type { enum print_type { PRINT_NORMAL, - PRINT_REVDEP_ALL, PRINT_REVDEP_YES, PRINT_REVDEP_MOD, PRINT_REVDEP_NO,