* [PATCH v3 0/3] Print reverse dependencies in groups
@ 2018-02-17 2:05 Eugeniu Rosca
2018-02-17 2:05 ` [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently Eugeniu Rosca
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Eugeniu Rosca @ 2018-02-17 2:05 UTC (permalink / raw)
To: Masahiro Yamada, Ulf Magnusson, Petr Vorel, Nicolas Pitre,
Randy Dunlap, Paul Bolle
Cc: Eugeniu Rosca, Eugeniu Rosca, linux-kbuild
From: Eugeniu Rosca <erosca@de.adit-jv.com>
Hello Masahiro, Ulf, Petr and all,
Here are a few words about the motivation behind this patch series.
First, the reason I got in touch with Kconfig is to optimize the
configuration of automotive kernels, as well as to align the kernel
configuration across a number of platforms. In the context of kernel
optimization, one of the primary goals is to filter out any features
that are not mentioned in the platform requirements.
Surprisingly or not, disabling a CONFIG option (which is assumed to
be unneeded) may be not so trivial. Especially it is not trivial, when
this CONFIG option is selected by a dozen of other configs. Before the
moment commit 1ccb27143360 ("kconfig: make "Selected by:" and
"Implied by:" readable") was submitted by Petr and eventually popped
up in v4.16-rc1, it was an absolute pain to break down the "Selected by"
reverse dependency expression in order to identify all those configs
which select (IOW *do not allow disabling*) a certain feature (assumed
to be not needed).
This patch series tries to make one step further and puts at users'
fingertips the revdep top level OR sub-expressions grouped/clustered by
the tristate value they evaluate to. This should allow the users to
directly concentrate on and tackle the active reverse dependencies,
which imho are the only ones that matter for a given ARCH and for a
given defconfig (nevertheless we still print all of them).
Changes v2->v3:
- Switch from reverse dependencies prefixed by their tristate value to
reverse dependencies grouped by the tristate value they evaluate to.
- Skip printing "{Selected,Implied} by [y|m|n]:" if there are no top
level OR tokens/sub-expressions that evaluate to y|m|n (suggested
by Petr).
- Use [1] as template for updating the interface/prototype of
__expr_print() (suggested by Ulf).
Changes v1->v2:
- Don't skip the =n reverse dependency OR tokens, since some users might
still need this information (suggested by Ulf).
- Instead of using "Selected by" for active tokens only, use it for all
OR tokens, but specify the tristate value of each token as prefix
(suggested by Masahiro).
[1] https://marc.info/?l=linux-kbuild&m=151777006005199&w=4
Eugeniu Rosca (3):
kconfig: Print reverse dependencies on new line consistently
kconfig: Prepare for printing reverse dependencies in groups
kconfig: Print reverse dependencies in groups
scripts/kconfig/expr.c | 118 ++++++++++++++++++++++++++++++++++++++------
scripts/kconfig/expr.h | 11 ++++-
scripts/kconfig/lkc_proto.h | 1 +
scripts/kconfig/menu.c | 37 ++++++++++----
4 files changed, 140 insertions(+), 27 deletions(-)
--
2.16.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently 2018-02-17 2:05 [PATCH v3 0/3] Print reverse dependencies in groups Eugeniu Rosca @ 2018-02-17 2:05 ` Eugeniu Rosca 2018-02-17 16:39 ` Ulf Magnusson 2018-02-17 2:05 ` [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups Eugeniu Rosca 2018-02-17 2:05 ` [PATCH v3 3/3] kconfig: Print " Eugeniu Rosca 2 siblings, 1 reply; 13+ messages in thread From: Eugeniu Rosca @ 2018-02-17 2:05 UTC (permalink / raw) To: Masahiro Yamada, Ulf Magnusson, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle Cc: Eugeniu Rosca, Eugeniu Rosca, linux-kbuild From: Eugeniu Rosca <erosca@de.adit-jv.com> Commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:" readable") made an incredible improvement in how reverse dependencies are perceived by the user, by breaking down the single (often interminable) expression string into small readable chunks, each of them displayed on a separate line: Selected by: - A && B - C && (D || E) Unfortunately, what happens with the non-OR (either E_SYMBOL or E_AND) expressions is that they don't get a dedicated line: Selected by: F && G That's arguably a bug/misbehavior, but it puts some amount of burden in implementing new ways of printing reverse dependencies to the user. As example, if we prefix every reverse dependency top level "||" token by its tristate value, then subjectively [2] looks more readable than [1]. [1] Selected by: [m] F && G [2] Selected by: - [m] F && G Also, if we print the reverse dependency sub-expressions in groups (clustered by the tristate value they evaluate to), then subjectively [4] looks more readable than [3]. [3] Selected by [m]: F && G [4] Selected by [m]: - F && G Based on the above, print all the reverse dependency sub-expressions on a separate line _consistently_. An example of change contributed by this commit is seen below. | Symbol: NEED_SG_DMA_LENGTH [=y] | ... | Selected by: IOMMU_DMA [=y] && IOMMU_SUPPORT [=y] becomes: | Symbol: NEED_SG_DMA_LENGTH [=y] | ... | Selected by: | - IOMMU_DMA [=y] && IOMMU_SUPPORT [=y] This patch has been tested using a tuned variant of zconfdump which prints the reverse dependencies for each config symbol. Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> Suggested-by: Ulf Magnusson <ulfalizer@gmail.com> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- scripts/kconfig/expr.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index d45381986ac7..b89baed7f15c 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1179,6 +1179,16 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2) return expr_get_leftmost_symbol(ret); } +static void +expr_print_newline(struct expr *e, + void (*fn)(void *, struct symbol *, const char *), + void *data, + int prevtoken) +{ + fn(data, NULL, "\n - "); + expr_print(e, fn, data, prevtoken); +} + static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep) { if (!e) { @@ -1191,7 +1201,10 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con switch (e->type) { case E_SYMBOL: if (e->left.sym->name) - fn(data, e->left.sym, e->left.sym->name); + if (!revdep) + fn(data, e->left.sym, e->left.sym->name); + else + expr_print_newline(e, fn, data, E_OR); else fn(data, NULL, "<choice>"); break; @@ -1234,19 +1247,19 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con fn(data, e->right.sym, e->right.sym->name); break; case E_OR: - if (revdep && e->left.expr->type != E_OR) - fn(data, NULL, "\n - "); __expr_print(e->left.expr, fn, data, E_OR, revdep); - if (revdep) - fn(data, NULL, "\n - "); - else + if (!revdep) fn(data, NULL, " || "); __expr_print(e->right.expr, fn, data, E_OR, revdep); break; case E_AND: - expr_print(e->left.expr, fn, data, E_AND); - fn(data, NULL, " && "); - expr_print(e->right.expr, fn, data, E_AND); + if (!revdep) { + expr_print(e->left.expr, fn, data, E_AND); + fn(data, NULL, " && "); + expr_print(e->right.expr, fn, data, E_AND); + } else { + expr_print_newline(e, fn, data, E_OR); + } break; case E_LIST: fn(data, e->right.sym, e->right.sym->name); -- 2.16.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently 2018-02-17 2:05 ` [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently Eugeniu Rosca @ 2018-02-17 16:39 ` Ulf Magnusson 0 siblings, 0 replies; 13+ messages in thread From: Ulf Magnusson @ 2018-02-17 16:39 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Eugeniu Rosca, Linux Kbuild mailing list On Sat, Feb 17, 2018 at 3:05 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > From: Eugeniu Rosca <erosca@de.adit-jv.com> > > Commit 1ccb27143360 ("kconfig: make "Selected by:" and "Implied by:" > readable") made an incredible improvement in how reverse dependencies > are perceived by the user, by breaking down the single (often > interminable) expression string into small readable chunks, each of > them displayed on a separate line: > > Selected by: > - A && B > - C && (D || E) > > Unfortunately, what happens with the non-OR (either E_SYMBOL or E_AND) > expressions is that they don't get a dedicated line: > > Selected by: F && G > > That's arguably a bug/misbehavior, but it puts some amount of burden in > implementing new ways of printing reverse dependencies to the user. As > example, if we prefix every reverse dependency top level "||" token by > its tristate value, then subjectively [2] looks more readable than [1]. > > [1] Selected by: [m] F && G > [2] Selected by: > - [m] F && G > > Also, if we print the reverse dependency sub-expressions in groups > (clustered by the tristate value they evaluate to), then > subjectively [4] looks more readable than [3]. > > [3] Selected by [m]: F && G > [4] Selected by [m]: > - F && G > > Based on the above, print all the reverse dependency sub-expressions on > a separate line _consistently_. An example of change contributed by this > commit is seen below. > > | Symbol: NEED_SG_DMA_LENGTH [=y] > | ... > | Selected by: IOMMU_DMA [=y] && IOMMU_SUPPORT [=y] > > becomes: > > | Symbol: NEED_SG_DMA_LENGTH [=y] > | ... > | Selected by: > | - IOMMU_DMA [=y] && IOMMU_SUPPORT [=y] > > This patch has been tested using a tuned variant of zconfdump which > prints the reverse dependencies for each config symbol. > > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Suggested-by: Ulf Magnusson <ulfalizer@gmail.com> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > scripts/kconfig/expr.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c > index d45381986ac7..b89baed7f15c 100644 > --- a/scripts/kconfig/expr.c > +++ b/scripts/kconfig/expr.c > @@ -1179,6 +1179,16 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2) > return expr_get_leftmost_symbol(ret); > } > > +static void > +expr_print_newline(struct expr *e, > + void (*fn)(void *, struct symbol *, const char *), > + void *data, > + int prevtoken) > +{ > + fn(data, NULL, "\n - "); > + expr_print(e, fn, data, prevtoken); > +} > + > static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep) > { > if (!e) { > @@ -1191,7 +1201,10 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con > switch (e->type) { > case E_SYMBOL: > if (e->left.sym->name) > - fn(data, e->left.sym, e->left.sym->name); > + if (!revdep) > + fn(data, e->left.sym, e->left.sym->name); > + else > + expr_print_newline(e, fn, data, E_OR); > else > fn(data, NULL, "<choice>"); > break; > @@ -1234,19 +1247,19 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con > fn(data, e->right.sym, e->right.sym->name); > break; > case E_OR: > - if (revdep && e->left.expr->type != E_OR) > - fn(data, NULL, "\n - "); > __expr_print(e->left.expr, fn, data, E_OR, revdep); > - if (revdep) > - fn(data, NULL, "\n - "); > - else > + if (!revdep) > fn(data, NULL, " || "); > __expr_print(e->right.expr, fn, data, E_OR, revdep); > break; > case E_AND: > - expr_print(e->left.expr, fn, data, E_AND); > - fn(data, NULL, " && "); > - expr_print(e->right.expr, fn, data, E_AND); > + if (!revdep) { > + expr_print(e->left.expr, fn, data, E_AND); > + fn(data, NULL, " && "); > + expr_print(e->right.expr, fn, data, E_AND); > + } else { > + expr_print_newline(e, fn, data, E_OR); > + } > break; > case E_LIST: > fn(data, e->right.sym, e->right.sym->name); > -- > 2.16.1 > Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> Cheers, Ulf ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups 2018-02-17 2:05 [PATCH v3 0/3] Print reverse dependencies in groups Eugeniu Rosca 2018-02-17 2:05 ` [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently Eugeniu Rosca @ 2018-02-17 2:05 ` Eugeniu Rosca 2018-02-17 16:44 ` Ulf Magnusson 2018-02-17 2:05 ` [PATCH v3 3/3] kconfig: Print " Eugeniu Rosca 2 siblings, 1 reply; 13+ messages in thread From: Eugeniu Rosca @ 2018-02-17 2:05 UTC (permalink / raw) To: Masahiro Yamada, Ulf Magnusson, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle Cc: Eugeniu Rosca, Eugeniu Rosca, linux-kbuild From: Eugeniu Rosca <erosca@de.adit-jv.com> Currently, reverse dependencies are printed like [1]. Prepare the ground for printing them like [2]. No functional change is intended in this patch. [1] Current pattern for printing reverse dependencies: Selected by: - EXPR_A_Y /* evaluates to =y */ - EXPR_B_N /* evaluates to =n */ - EXPR_C_Y /* evaluates to =y */ - EXPR_D_M /* evaluates to =m */ - EXPR_E_N /* evaluates to =n */ [2] Upcoming pattern for printing reverse dependencies: Selected by [y]: - EXPR_A_Y - EXPR_C_Y Selected by [m]: - EXPR_D_M Selected by [n]: - EXPR_B_N - EXPR_E_N Suggested-by: Ulf Magnusson <ulfalizer@gmail.com> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- scripts/kconfig/expr.c | 38 +++++++++++++++++++++++++++----------- scripts/kconfig/expr.h | 7 ++++++- scripts/kconfig/menu.c | 4 ++-- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index b89baed7f15c..c610cb14284f 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1189,7 +1189,12 @@ expr_print_newline(struct expr *e, expr_print(e, fn, data, prevtoken); } -static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep) +static void +__expr_print(struct expr *e, + void (*fn)(void *, struct symbol *, const char *), + void *data, + int prevtoken, + enum print_type type) { if (!e) { fn(data, NULL, "y"); @@ -1201,10 +1206,16 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con switch (e->type) { case E_SYMBOL: if (e->left.sym->name) - if (!revdep) + switch (type) { + case PRINT_NORMAL: fn(data, e->left.sym, e->left.sym->name); - else + break; + case PRINT_REVDEP_ALL: expr_print_newline(e, fn, data, E_OR); + break; + default: + break; + } else fn(data, NULL, "<choice>"); break; @@ -1247,18 +1258,23 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con fn(data, e->right.sym, e->right.sym->name); break; case E_OR: - __expr_print(e->left.expr, fn, data, E_OR, revdep); - if (!revdep) + __expr_print(e->left.expr, fn, data, E_OR, type); + if (type == PRINT_NORMAL) fn(data, NULL, " || "); - __expr_print(e->right.expr, fn, data, E_OR, revdep); + __expr_print(e->right.expr, fn, data, E_OR, type); break; case E_AND: - if (!revdep) { + switch (type) { + case PRINT_NORMAL: expr_print(e->left.expr, fn, data, E_AND); fn(data, NULL, " && "); expr_print(e->right.expr, fn, data, E_AND); - } else { + break; + case PRINT_REVDEP_ALL: expr_print_newline(e, fn, data, E_OR); + break; + default: + break; } break; case E_LIST: @@ -1289,7 +1305,7 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken) { - __expr_print(e, fn, data, prevtoken, false); + __expr_print(e, fn, data, prevtoken, PRINT_NORMAL); } static void expr_print_file_helper(void *data, struct symbol *sym, const char *str) @@ -1342,7 +1358,7 @@ void expr_gstr_print(struct expr *e, struct gstr *gs) * line with a minus. This makes expressions much easier to read. * Suitable for reverse dependency expressions. */ -void expr_gstr_print_revdep(struct expr *e, struct gstr *gs) +void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t) { - __expr_print(e, expr_print_gstr_helper, gs, E_NONE, true); + __expr_print(e, expr_print_gstr_helper, gs, E_NONE, t); } diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index c16e82e302a2..21cb67c15091 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -34,6 +34,11 @@ enum expr_type { E_LIST, E_SYMBOL, E_RANGE }; +enum print_type { + PRINT_NORMAL, + PRINT_REVDEP_ALL, +}; + union expr_data { struct expr *expr; struct symbol *sym; @@ -310,7 +315,7 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2); void expr_fprint(struct expr *e, FILE *out); struct gstr; /* forward */ void expr_gstr_print(struct expr *e, struct gstr *gs); -void expr_gstr_print_revdep(struct expr *e, struct gstr *gs); +void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t); static inline int expr_is_yes(struct expr *e) { diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index 99222855544c..5b8edba105f2 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -828,14 +828,14 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, get_symbol_props_str(r, sym, P_SELECT, _(" Selects: ")); if (sym->rev_dep.expr) { str_append(r, _(" Selected by: ")); - expr_gstr_print_revdep(sym->rev_dep.expr, r); + expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL); str_append(r, "\n"); } get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); if (sym->implied.expr) { str_append(r, _(" Implied by: ")); - expr_gstr_print_revdep(sym->implied.expr, r); + expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL); str_append(r, "\n"); } -- 2.16.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups 2018-02-17 2:05 ` [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups Eugeniu Rosca @ 2018-02-17 16:44 ` Ulf Magnusson 0 siblings, 0 replies; 13+ messages in thread From: Ulf Magnusson @ 2018-02-17 16:44 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Eugeniu Rosca, Linux Kbuild mailing list On Sat, Feb 17, 2018 at 3:05 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > From: Eugeniu Rosca <erosca@de.adit-jv.com> > > Currently, reverse dependencies are printed like [1]. Prepare the ground > for printing them like [2]. No functional change is intended in this > patch. > > [1] Current pattern for printing reverse dependencies: > Selected by: > - EXPR_A_Y /* evaluates to =y */ > - EXPR_B_N /* evaluates to =n */ > - EXPR_C_Y /* evaluates to =y */ > - EXPR_D_M /* evaluates to =m */ > - EXPR_E_N /* evaluates to =n */ > > [2] Upcoming pattern for printing reverse dependencies: > Selected by [y]: > - EXPR_A_Y > - EXPR_C_Y > Selected by [m]: > - EXPR_D_M > Selected by [n]: > - EXPR_B_N > - EXPR_E_N > > Suggested-by: Ulf Magnusson <ulfalizer@gmail.com> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > scripts/kconfig/expr.c | 38 +++++++++++++++++++++++++++----------- > scripts/kconfig/expr.h | 7 ++++++- > scripts/kconfig/menu.c | 4 ++-- > 3 files changed, 35 insertions(+), 14 deletions(-) > > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c > index b89baed7f15c..c610cb14284f 100644 > --- a/scripts/kconfig/expr.c > +++ b/scripts/kconfig/expr.c > @@ -1189,7 +1189,12 @@ expr_print_newline(struct expr *e, > expr_print(e, fn, data, prevtoken); > } > > -static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken, bool revdep) > +static void > +__expr_print(struct expr *e, > + void (*fn)(void *, struct symbol *, const char *), > + void *data, > + int prevtoken, > + enum print_type type) > { > if (!e) { > fn(data, NULL, "y"); > @@ -1201,10 +1206,16 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con > switch (e->type) { > case E_SYMBOL: > if (e->left.sym->name) > - if (!revdep) > + switch (type) { > + case PRINT_NORMAL: > fn(data, e->left.sym, e->left.sym->name); > - else > + break; > + case PRINT_REVDEP_ALL: > expr_print_newline(e, fn, data, E_OR); > + break; > + default: > + break; Redundant 'default' case. > + } > else > fn(data, NULL, "<choice>"); > break; > @@ -1247,18 +1258,23 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con > fn(data, e->right.sym, e->right.sym->name); > break; > case E_OR: > - __expr_print(e->left.expr, fn, data, E_OR, revdep); > - if (!revdep) > + __expr_print(e->left.expr, fn, data, E_OR, type); > + if (type == PRINT_NORMAL) > fn(data, NULL, " || "); > - __expr_print(e->right.expr, fn, data, E_OR, revdep); > + __expr_print(e->right.expr, fn, data, E_OR, type); > break; > case E_AND: > - if (!revdep) { > + switch (type) { > + case PRINT_NORMAL: > expr_print(e->left.expr, fn, data, E_AND); > fn(data, NULL, " && "); > expr_print(e->right.expr, fn, data, E_AND); > - } else { > + break; > + case PRINT_REVDEP_ALL: > expr_print_newline(e, fn, data, E_OR); > + break; > + default: > + break; Redundant 'default' case. > } > break; > case E_LIST: > @@ -1289,7 +1305,7 @@ static void __expr_print(struct expr *e, void (*fn)(void *, struct symbol *, con > > void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken) > { > - __expr_print(e, fn, data, prevtoken, false); > + __expr_print(e, fn, data, prevtoken, PRINT_NORMAL); > } > > static void expr_print_file_helper(void *data, struct symbol *sym, const char *str) > @@ -1342,7 +1358,7 @@ void expr_gstr_print(struct expr *e, struct gstr *gs) > * line with a minus. This makes expressions much easier to read. > * Suitable for reverse dependency expressions. > */ > -void expr_gstr_print_revdep(struct expr *e, struct gstr *gs) > +void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t) > { > - __expr_print(e, expr_print_gstr_helper, gs, E_NONE, true); > + __expr_print(e, expr_print_gstr_helper, gs, E_NONE, t); > } > diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h > index c16e82e302a2..21cb67c15091 100644 > --- a/scripts/kconfig/expr.h > +++ b/scripts/kconfig/expr.h > @@ -34,6 +34,11 @@ enum expr_type { > E_LIST, E_SYMBOL, E_RANGE > }; > > +enum print_type { > + PRINT_NORMAL, > + PRINT_REVDEP_ALL, > +}; > + > union expr_data { > struct expr *expr; > struct symbol *sym; > @@ -310,7 +315,7 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2); > void expr_fprint(struct expr *e, FILE *out); > struct gstr; /* forward */ > void expr_gstr_print(struct expr *e, struct gstr *gs); > -void expr_gstr_print_revdep(struct expr *e, struct gstr *gs); > +void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t); > > static inline int expr_is_yes(struct expr *e) > { > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c > index 99222855544c..5b8edba105f2 100644 > --- a/scripts/kconfig/menu.c > +++ b/scripts/kconfig/menu.c > @@ -828,14 +828,14 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, > get_symbol_props_str(r, sym, P_SELECT, _(" Selects: ")); > if (sym->rev_dep.expr) { > str_append(r, _(" Selected by: ")); > - expr_gstr_print_revdep(sym->rev_dep.expr, r); > + expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL); > str_append(r, "\n"); > } > > get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); > if (sym->implied.expr) { > str_append(r, _(" Implied by: ")); > - expr_gstr_print_revdep(sym->implied.expr, r); > + expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL); > str_append(r, "\n"); > } > > -- > 2.16.1 > I'm not a fan of adding defaults in switches that already cover the whole range of values (except to have an assert() in there maybe), but it feels a bit nitpicky. Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> Cheers, Ulf ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-17 2:05 [PATCH v3 0/3] Print reverse dependencies in groups Eugeniu Rosca 2018-02-17 2:05 ` [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently Eugeniu Rosca 2018-02-17 2:05 ` [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups Eugeniu Rosca @ 2018-02-17 2:05 ` Eugeniu Rosca 2018-02-17 16:55 ` Ulf Magnusson 2 siblings, 1 reply; 13+ messages in thread From: Eugeniu Rosca @ 2018-02-17 2:05 UTC (permalink / raw) To: Masahiro Yamada, Ulf Magnusson, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle Cc: Eugeniu Rosca, Eugeniu Rosca, linux-kbuild From: Eugeniu Rosca <erosca@de.adit-jv.com> Assuming commit 617aebe6a97e ("Merge tag 'usercopy-v4.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux"), ARCH=arm64 and vanilla arm64 defconfig, here is the top 10 CONFIG options with the highest amount of top level "||" sub-expressions/tokens that make up the final "{Selected,Implied} by" reverse dependency expression. | Config | Revdep all | Revdep ![=n] | |-------------------------------|------------|--------------| | REGMAP_I2C | 212 | 9 | | CRC32 | 167 | 25 | | FW_LOADER | 128 | 5 | | MFD_CORE | 124 | 9 | | FB_CFB_IMAGEBLIT | 114 | 2 | | FB_CFB_COPYAREA | 111 | 2 | | FB_CFB_FILLRECT | 110 | 2 | | SND_PCM | 103 | 2 | | CRYPTO_HASH | 87 | 19 | | WATCHDOG_CORE | 86 | 6 | The story behind the above is that users need to visually review/evaluate 212 expressions which *potentially* select REGMAP_I2C in order to identify the expressions which *actually* select REGMAP_I2C, for a particular ARCH and for a particular defconfig used. To make this experience smoother, change the way reverse dependencies are displayed to the user from [1] to [2]. [1] Old representation of reverse dependencies for DMA_ENGINE_RAID: Selected by: - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || 440SP) - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] [2] New representation of reverse dependencies for DMA_ENGINE_RAID: Selected by [y]: - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] Selected by [m]: - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... Selected by [n]: - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ... - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- scripts/kconfig/expr.c | 59 ++++++++++++++++++++++++++++++++++++++++++++- scripts/kconfig/expr.h | 4 +++ scripts/kconfig/lkc_proto.h | 1 + scripts/kconfig/menu.c | 37 ++++++++++++++++++++-------- 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index c610cb14284f..48b99371d276 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1213,6 +1213,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; 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; 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_YES, + PRINT_REVDEP_MOD, + PRINT_REVDEP_NO, }; union expr_data { @@ -316,6 +319,7 @@ void expr_fprint(struct expr *e, FILE *out); struct gstr; /* forward */ void expr_gstr_print(struct expr *e, struct gstr *gs); void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t); +bool expr_revdep_contains(struct expr *e, tristate val); static inline int expr_is_yes(struct expr *e) { diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h index 9dc8abfb1dc3..69ed1477e4ef 100644 --- a/scripts/kconfig/lkc_proto.h +++ b/scripts/kconfig/lkc_proto.h @@ -25,6 +25,7 @@ bool menu_has_help(struct menu *menu); const char * menu_get_help(struct menu *menu); struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head); void menu_get_ext_help(struct menu *menu, struct gstr *help); +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r); /* symbol.c */ extern struct symbol * symbol_hash[SYMBOL_HASHSIZE]; diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index 5b8edba105f2..d13ffa69d65b 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -790,6 +790,31 @@ static void get_symbol_props_str(struct gstr *r, struct symbol *sym, str_append(r, "\n"); } +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r) +{ + if (!e) + return; + + if (expr_revdep_contains(e, yes)) { + str_append(r, s); + str_append(r, _(" [y]:")); + expr_gstr_print_revdep(e, r, PRINT_REVDEP_YES); + str_append(r, "\n"); + } + if (expr_revdep_contains(e, mod)) { + str_append(r, s); + str_append(r, _(" [m]:")); + expr_gstr_print_revdep(e, r, PRINT_REVDEP_MOD); + str_append(r, "\n"); + } + if (expr_revdep_contains(e, no)) { + str_append(r, s); + str_append(r, _(" [n]:")); + expr_gstr_print_revdep(e, r, PRINT_REVDEP_NO); + str_append(r, "\n"); + } +} + /* * head is optional and may be NULL */ @@ -826,18 +851,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, } get_symbol_props_str(r, sym, P_SELECT, _(" Selects: ")); - if (sym->rev_dep.expr) { - str_append(r, _(" Selected by: ")); - expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL); - str_append(r, "\n"); - } + get_revdep_by_type(sym->rev_dep.expr, " Selected by", r); get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); - if (sym->implied.expr) { - str_append(r, _(" Implied by: ")); - expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL); - str_append(r, "\n"); - } + get_revdep_by_type(sym->implied.expr, " Implied by", r); str_append(r, "\n\n"); } -- 2.16.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-17 2:05 ` [PATCH v3 3/3] kconfig: Print " Eugeniu Rosca @ 2018-02-17 16:55 ` Ulf Magnusson 2018-02-18 11:07 ` Eugeniu Rosca 0 siblings, 1 reply; 13+ messages in thread From: Ulf Magnusson @ 2018-02-17 16:55 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Eugeniu Rosca, Linux Kbuild mailing list On Sat, Feb 17, 2018 at 3:05 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > From: Eugeniu Rosca <erosca@de.adit-jv.com> > > Assuming commit 617aebe6a97e ("Merge tag 'usercopy-v4.16-rc1' of > git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux"), ARCH=arm64 > and vanilla arm64 defconfig, here is the top 10 CONFIG options with > the highest amount of top level "||" sub-expressions/tokens that make > up the final "{Selected,Implied} by" reverse dependency expression. > > | Config | Revdep all | Revdep ![=n] | > |-------------------------------|------------|--------------| > | REGMAP_I2C | 212 | 9 | > | CRC32 | 167 | 25 | > | FW_LOADER | 128 | 5 | > | MFD_CORE | 124 | 9 | > | FB_CFB_IMAGEBLIT | 114 | 2 | > | FB_CFB_COPYAREA | 111 | 2 | > | FB_CFB_FILLRECT | 110 | 2 | > | SND_PCM | 103 | 2 | > | CRYPTO_HASH | 87 | 19 | > | WATCHDOG_CORE | 86 | 6 | > > The story behind the above is that users need to visually > review/evaluate 212 expressions which *potentially* select REGMAP_I2C > in order to identify the expressions which *actually* select REGMAP_I2C, > for a particular ARCH and for a particular defconfig used. > > To make this experience smoother, change the way reverse dependencies > are displayed to the user from [1] to [2]. > > [1] Old representation of reverse dependencies for DMA_ENGINE_RAID: > Selected by: > - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || 440SP) > - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... > - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... > - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 > - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... > - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] > - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... > - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] > > [2] New representation of reverse dependencies for DMA_ENGINE_RAID: > Selected by [y]: > - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] > Selected by [m]: > - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... > Selected by [n]: > - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ... > - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... > - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 > - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... > - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... > - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > scripts/kconfig/expr.c | 59 ++++++++++++++++++++++++++++++++++++++++++++- > scripts/kconfig/expr.h | 4 +++ > scripts/kconfig/lkc_proto.h | 1 + > scripts/kconfig/menu.c | 37 ++++++++++++++++++++-------- > 4 files changed, 90 insertions(+), 11 deletions(-) > > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c > index c610cb14284f..48b99371d276 100644 > --- a/scripts/kconfig/expr.c > +++ b/scripts/kconfig/expr.c > @@ -1213,6 +1213,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; Perhaps this logic could be moved into expr_print_newline() (which could be renamed to something like expr_print_select() in that case). Could have it take 'enum print_type' as an argument. > 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. > + PRINT_REVDEP_YES, > + PRINT_REVDEP_MOD, > + PRINT_REVDEP_NO, > }; > > union expr_data { > @@ -316,6 +319,7 @@ void expr_fprint(struct expr *e, FILE *out); > struct gstr; /* forward */ > void expr_gstr_print(struct expr *e, struct gstr *gs); > void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t); > +bool expr_revdep_contains(struct expr *e, tristate val); > > static inline int expr_is_yes(struct expr *e) > { > diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h > index 9dc8abfb1dc3..69ed1477e4ef 100644 > --- a/scripts/kconfig/lkc_proto.h > +++ b/scripts/kconfig/lkc_proto.h > @@ -25,6 +25,7 @@ bool menu_has_help(struct menu *menu); > const char * menu_get_help(struct menu *menu); > struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head); > void menu_get_ext_help(struct menu *menu, struct gstr *help); > +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r); > > /* symbol.c */ > extern struct symbol * symbol_hash[SYMBOL_HASHSIZE]; > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c > index 5b8edba105f2..d13ffa69d65b 100644 > --- a/scripts/kconfig/menu.c > +++ b/scripts/kconfig/menu.c > @@ -790,6 +790,31 @@ static void get_symbol_props_str(struct gstr *r, struct symbol *sym, > str_append(r, "\n"); > } > > +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r) > +{ > + if (!e) > + return; > + > + if (expr_revdep_contains(e, yes)) { > + str_append(r, s); > + str_append(r, _(" [y]:")); > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_YES); > + str_append(r, "\n"); > + } > + if (expr_revdep_contains(e, mod)) { > + str_append(r, s); > + str_append(r, _(" [m]:")); > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_MOD); > + str_append(r, "\n"); > + } > + if (expr_revdep_contains(e, no)) { > + str_append(r, s); > + str_append(r, _(" [n]:")); > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_NO); > + str_append(r, "\n"); > + } > +} Those _() are for gettext btw. Not sure those strings need translations (or if anyone is translating Kconfig), so I think they could be removed here. > + > /* > * head is optional and may be NULL > */ > @@ -826,18 +851,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, > } > > get_symbol_props_str(r, sym, P_SELECT, _(" Selects: ")); > - if (sym->rev_dep.expr) { > - str_append(r, _(" Selected by: ")); > - expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL); > - str_append(r, "\n"); > - } > + get_revdep_by_type(sym->rev_dep.expr, " Selected by", r); > > get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); > - if (sym->implied.expr) { > - str_append(r, _(" Implied by: ")); > - expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL); > - str_append(r, "\n"); > - } > + get_revdep_by_type(sym->implied.expr, " Implied by", r); > > str_append(r, "\n\n"); > } > -- > 2.16.1 > Looks like a very nice patchset in general to me. Cheers, Ulf ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-17 16:55 ` Ulf Magnusson @ 2018-02-18 11:07 ` Eugeniu Rosca 2018-02-18 13:02 ` Ulf Magnusson 0 siblings, 1 reply; 13+ messages in thread From: Eugeniu Rosca @ 2018-02-18 11:07 UTC (permalink / raw) To: Ulf Magnusson Cc: Eugeniu Rosca, Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Linux Kbuild mailing list On Sat, Feb 17, 2018 at 05:55:01PM +0100, Ulf Magnusson wrote: > On Sat, Feb 17, 2018 at 3:05 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > From: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > Assuming commit 617aebe6a97e ("Merge tag 'usercopy-v4.16-rc1' of > > git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux"), ARCH=arm64 > > and vanilla arm64 defconfig, here is the top 10 CONFIG options with > > the highest amount of top level "||" sub-expressions/tokens that make > > up the final "{Selected,Implied} by" reverse dependency expression. > > > > | Config | Revdep all | Revdep ![=n] | > > |-------------------------------|------------|--------------| > > | REGMAP_I2C | 212 | 9 | > > | CRC32 | 167 | 25 | > > | FW_LOADER | 128 | 5 | > > | MFD_CORE | 124 | 9 | > > | FB_CFB_IMAGEBLIT | 114 | 2 | > > | FB_CFB_COPYAREA | 111 | 2 | > > | FB_CFB_FILLRECT | 110 | 2 | > > | SND_PCM | 103 | 2 | > > | CRYPTO_HASH | 87 | 19 | > > | WATCHDOG_CORE | 86 | 6 | > > > > The story behind the above is that users need to visually > > review/evaluate 212 expressions which *potentially* select REGMAP_I2C > > in order to identify the expressions which *actually* select REGMAP_I2C, > > for a particular ARCH and for a particular defconfig used. > > > > To make this experience smoother, change the way reverse dependencies > > are displayed to the user from [1] to [2]. > > > > [1] Old representation of reverse dependencies for DMA_ENGINE_RAID: > > Selected by: > > - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || 440SP) > > - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... > > - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... > > - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 > > - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... > > - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] > > - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... > > - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] > > > > [2] New representation of reverse dependencies for DMA_ENGINE_RAID: > > Selected by [y]: > > - MV_XOR_V2 [=y] && DMADEVICES [=y] && ARM64 [=y] > > Selected by [m]: > > - BCM_SBA_RAID [=m] && DMADEVICES [=y] && (ARM64 [=y] || ... > > Selected by [n]: > > - AMCC_PPC440SPE_ADMA [=n] && DMADEVICES [=y] && (440SPe || ... > > - FSL_RAID [=n] && DMADEVICES [=y] && FSL_SOC && ... > > - INTEL_IOATDMA [=n] && DMADEVICES [=y] && PCI [=y] && X86_64 > > - MV_XOR [=n] && DMADEVICES [=y] && (PLAT_ORION || ARCH_MVEBU [=y] ... > > - XGENE_DMA [=n] && DMADEVICES [=y] && (ARCH_XGENE [=y] || ... > > - DMATEST [=n] && DMADEVICES [=y] && DMA_ENGINE [=y] > > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > --- > > scripts/kconfig/expr.c | 59 ++++++++++++++++++++++++++++++++++++++++++++- > > scripts/kconfig/expr.h | 4 +++ > > scripts/kconfig/lkc_proto.h | 1 + > > scripts/kconfig/menu.c | 37 ++++++++++++++++++++-------- > > 4 files changed, 90 insertions(+), 11 deletions(-) > > > > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c > > index c610cb14284f..48b99371d276 100644 > > --- a/scripts/kconfig/expr.c > > +++ b/scripts/kconfig/expr.c > > @@ -1213,6 +1213,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; > > Perhaps this logic could be moved into expr_print_newline() (which > could be renamed to something like expr_print_select() in that case). > Could have it take 'enum print_type' as an argument. 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; + } +} + 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, > > > + PRINT_REVDEP_YES, > > + PRINT_REVDEP_MOD, > > + PRINT_REVDEP_NO, > > }; > > > > union expr_data { > > @@ -316,6 +319,7 @@ void expr_fprint(struct expr *e, FILE *out); > > struct gstr; /* forward */ > > void expr_gstr_print(struct expr *e, struct gstr *gs); > > void expr_gstr_print_revdep(struct expr *e, struct gstr *gs, enum print_type t); > > +bool expr_revdep_contains(struct expr *e, tristate val); > > > > static inline int expr_is_yes(struct expr *e) > > { > > diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h > > index 9dc8abfb1dc3..69ed1477e4ef 100644 > > --- a/scripts/kconfig/lkc_proto.h > > +++ b/scripts/kconfig/lkc_proto.h > > @@ -25,6 +25,7 @@ bool menu_has_help(struct menu *menu); > > const char * menu_get_help(struct menu *menu); > > struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head); > > void menu_get_ext_help(struct menu *menu, struct gstr *help); > > +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r); > > > > /* symbol.c */ > > extern struct symbol * symbol_hash[SYMBOL_HASHSIZE]; > > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c > > index 5b8edba105f2..d13ffa69d65b 100644 > > --- a/scripts/kconfig/menu.c > > +++ b/scripts/kconfig/menu.c > > @@ -790,6 +790,31 @@ static void get_symbol_props_str(struct gstr *r, struct symbol *sym, > > str_append(r, "\n"); > > } > > > > +void get_revdep_by_type(struct expr *e, char *s, struct gstr *r) > > +{ > > + if (!e) > > + return; > > + > > + if (expr_revdep_contains(e, yes)) { > > + str_append(r, s); > > + str_append(r, _(" [y]:")); > > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_YES); > > + str_append(r, "\n"); > > + } > > + if (expr_revdep_contains(e, mod)) { > > + str_append(r, s); > > + str_append(r, _(" [m]:")); > > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_MOD); > > + str_append(r, "\n"); > > + } > > + if (expr_revdep_contains(e, no)) { > > + str_append(r, s); > > + str_append(r, _(" [n]:")); > > + expr_gstr_print_revdep(e, r, PRINT_REVDEP_NO); > > + str_append(r, "\n"); > > + } > > +} > > Those _() are for gettext btw. Not sure those strings need > translations (or if anyone is translating Kconfig), so I think they > could be removed here. No problem. I can remove them :) > > > + > > /* > > * head is optional and may be NULL > > */ > > @@ -826,18 +851,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym, > > } > > > > get_symbol_props_str(r, sym, P_SELECT, _(" Selects: ")); > > - if (sym->rev_dep.expr) { > > - str_append(r, _(" Selected by: ")); > > - expr_gstr_print_revdep(sym->rev_dep.expr, r, PRINT_REVDEP_ALL); > > - str_append(r, "\n"); > > - } > > + get_revdep_by_type(sym->rev_dep.expr, " Selected by", r); > > > > get_symbol_props_str(r, sym, P_IMPLY, _(" Implies: ")); > > - if (sym->implied.expr) { > > - str_append(r, _(" Implied by: ")); > > - expr_gstr_print_revdep(sym->implied.expr, r, PRINT_REVDEP_ALL); > > - str_append(r, "\n"); > > - } > > + get_revdep_by_type(sym->implied.expr, " Implied by", r); > > > > str_append(r, "\n\n"); > > } > > -- > > 2.16.1 > > > > Looks like a very nice patchset in general to me. After getting your feedback/preference on the above, I will hopefully be able to push v4. Thanks for the reviewing effort! > Cheers, > Ulf Regards, Eugeniu. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-18 11:07 ` Eugeniu Rosca @ 2018-02-18 13:02 ` Ulf Magnusson 2018-02-18 13:19 ` Ulf Magnusson 0 siblings, 1 reply; 13+ messages in thread From: Ulf Magnusson @ 2018-02-18 13:02 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Linux Kbuild mailing list 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-18 13:02 ` Ulf Magnusson @ 2018-02-18 13:19 ` Ulf Magnusson 2018-02-18 13:35 ` Ulf Magnusson 0 siblings, 1 reply; 13+ messages in thread From: Ulf Magnusson @ 2018-02-18 13:19 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Linux Kbuild mailing list 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-18 13:19 ` Ulf Magnusson @ 2018-02-18 13:35 ` Ulf Magnusson 2018-02-18 13:40 ` Ulf Magnusson 0 siblings, 1 reply; 13+ messages in thread From: Ulf Magnusson @ 2018-02-18 13:35 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Linux Kbuild mailing list 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-18 13:35 ` Ulf Magnusson @ 2018-02-18 13:40 ` Ulf Magnusson 2018-02-18 21:05 ` Eugeniu Rosca 0 siblings, 1 reply; 13+ messages in thread From: Ulf Magnusson @ 2018-02-18 13:40 UTC (permalink / raw) To: Eugeniu Rosca Cc: Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Eugeniu Rosca, Linux Kbuild mailing list 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups 2018-02-18 13:40 ` Ulf Magnusson @ 2018-02-18 21:05 ` Eugeniu Rosca 0 siblings, 0 replies; 13+ messages in thread From: Eugeniu Rosca @ 2018-02-18 21:05 UTC (permalink / raw) To: Ulf Magnusson Cc: Eugeniu Rosca, Masahiro Yamada, Petr Vorel, Nicolas Pitre, Randy Dunlap, Paul Bolle, Linux Kbuild mailing list, Eugeniu Rosca Hi Ulf, Sometimes having so many options (all of them being equally clean, simple and elegant) can make the choice really hard :D Below is the interdiff between v3 and v4. Hopefully other people will also like this series and we will see it in in some weeks. Thank you very much for your support. diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c index 48b99371d276..95dc058a236f 100644 --- a/scripts/kconfig/expr.c +++ b/scripts/kconfig/expr.c @@ -1180,13 +1180,19 @@ struct expr *expr_simplify_unmet_dep(struct expr *e1, struct expr *e2) } static void -expr_print_newline(struct expr *e, - void (*fn)(void *, struct symbol *, const char *), - void *data, - int prevtoken) +expr_print_revdep(struct expr *e, + void (*fn)(void *, struct symbol *, const char *), + void *data, + int prevtoken, + enum print_type type) { - fn(data, NULL, "\n - "); - expr_print(e, fn, data, prevtoken); + 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); + } } static void @@ -1211,21 +1217,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 +1278,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; diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index d13ffa69d65b..029da77fe1b0 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -797,19 +797,19 @@ void get_revdep_by_type(struct expr *e, char *s, struct gstr *r) if (expr_revdep_contains(e, yes)) { str_append(r, s); - str_append(r, _(" [y]:")); + str_append(r, " [y]:"); expr_gstr_print_revdep(e, r, PRINT_REVDEP_YES); str_append(r, "\n"); } if (expr_revdep_contains(e, mod)) { str_append(r, s); - str_append(r, _(" [m]:")); + str_append(r, " [m]:"); expr_gstr_print_revdep(e, r, PRINT_REVDEP_MOD); str_append(r, "\n"); } if (expr_revdep_contains(e, no)) { str_append(r, s); - str_append(r, _(" [n]:")); + str_append(r, " [n]:"); expr_gstr_print_revdep(e, r, PRINT_REVDEP_NO); str_append(r, "\n"); } Best regards, Eugeniu. ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-02-18 21:05 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-17 2:05 [PATCH v3 0/3] Print reverse dependencies in groups Eugeniu Rosca 2018-02-17 2:05 ` [PATCH v3 1/3] kconfig: Print reverse dependencies on new line consistently Eugeniu Rosca 2018-02-17 16:39 ` Ulf Magnusson 2018-02-17 2:05 ` [PATCH v3 2/3] kconfig: Prepare for printing reverse dependencies in groups Eugeniu Rosca 2018-02-17 16:44 ` Ulf Magnusson 2018-02-17 2:05 ` [PATCH v3 3/3] kconfig: Print " Eugeniu Rosca 2018-02-17 16:55 ` Ulf Magnusson 2018-02-18 11:07 ` Eugeniu Rosca 2018-02-18 13:02 ` Ulf Magnusson 2018-02-18 13:19 ` Ulf Magnusson 2018-02-18 13:35 ` Ulf Magnusson 2018-02-18 13:40 ` Ulf Magnusson 2018-02-18 21:05 ` Eugeniu Rosca
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.