From: Eugeniu Rosca <roscaeugeniu@gmail.com>
To: Ulf Magnusson <ulfalizer@gmail.com>
Cc: Eugeniu Rosca <roscaeugeniu@gmail.com>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
Petr Vorel <petr.vorel@gmail.com>,
Nicolas Pitre <nicolas.pitre@linaro.org>,
Randy Dunlap <rdunlap@infradead.org>,
Paul Bolle <pebolle@tiscali.nl>,
Eugeniu Rosca <erosca@de.adit-jv.com>,
Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>
Subject: Re: [PATCH v3 3/3] kconfig: Print reverse dependencies in groups
Date: Sun, 18 Feb 2018 12:07:02 +0100 [thread overview]
Message-ID: <20180218110702.GA26185@example.com> (raw)
In-Reply-To: <CAFkk2KT8gQz+wmMagq-v4zLX7XuZ6MahN8KQ6SqtaAMRo5o7sA@mail.gmail.com>
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.
next prev parent reply other threads:[~2018-02-18 11:07 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180218110702.GA26185@example.com \
--to=roscaeugeniu@gmail.com \
--cc=erosca@de.adit-jv.com \
--cc=linux-kbuild@vger.kernel.org \
--cc=nicolas.pitre@linaro.org \
--cc=pebolle@tiscali.nl \
--cc=petr.vorel@gmail.com \
--cc=rdunlap@infradead.org \
--cc=ulfalizer@gmail.com \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.