Hi Ulf,
On Tue, Feb 13, 2018 at 07:18:27AM +0100, Ulf Magnusson wrote:
> On Tue, Feb 13, 2018 at 1:56 AM, Eugeniu Rosca <roscaeugeniu@xxxxxxxxx> wrote:
> > From: Eugeniu Rosca <erosca@xxxxxxxxxxxxxx>
> >
> > 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 OR sub-expressions 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 table is that the user needs 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, transform the way reverse dependencies
> > are displayed to the user from [1] to [2].
> >
> > [1] Before this commit
> > Symbol: MTD_BLKDEVS [=y]
> > ...
> > Selected by:
> > - MTD_BLOCK [=y] && MTD [=y] && BLOCK [=y]
> > - MTD_BLOCK_RO [=n] && MTD [=y] && MTD_BLOCK [=y]!=y && BLOCK [=y]
> > - FTL [=n] && MTD [=y] && BLOCK [=y]
> > - NFTL [=n] && MTD [=y] && BLOCK [=y]
> > - INFTL [=n] && MTD [=y] && BLOCK [=y]
> > - RFD_FTL [=n] && MTD [=y] && BLOCK [=y]
> > - SSFDC [=n] && MTD [=y] && BLOCK [=y]
> > - SM_FTL [=n] && MTD [=y] && BLOCK [=y]
> > - MTD_SWAP [=n] && MTD [=y] && SWAP [=y]
> >
> > [2] After this commit
> > Symbol: MTD_BLKDEVS [=y]
> > ...
> > Selected by:
> > - [y] MTD_BLOCK [=y] && MTD [=y] && BLOCK [=y]
> > - [ ] MTD_BLOCK_RO [=n] && MTD [=y] && MTD_BLOCK [=y]!=y && BLOCK [=y]
> > - [ ] FTL [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] NFTL [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] INFTL [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] RFD_FTL [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] SSFDC [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] SM_FTL [=n] && MTD [=y] && BLOCK [=y]
> > - [ ] MTD_SWAP [=n] && MTD [=y] && SWAP [=y]
> >
> > This patch has been tested using a custom variant of zconfdump which
> > prints the reverse dependencies for each config symbol.
> >
> > Suggested-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> > Suggested-by: Ulf Magnusson <ulfalizer@xxxxxxxxx>
> > Signed-off-by: Eugeniu Rosca <erosca@xxxxxxxxxxxxxx>
> > ---
> > scripts/kconfig/expr.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
> > index 0704bcdf4c78..173fc5b252d5 100644
> > --- a/scripts/kconfig/expr.c
> > +++ b/scripts/kconfig/expr.c
> > @@ -1185,7 +1185,19 @@ expr_print_newline(struct expr *e,
> > void *data,
> > int prevtoken)
> > {
> > - fn(data, NULL, "\n - ");
> > + switch (expr_calc_value(e)) {
> > + case yes:
> > + fn(data, NULL, "\n - [y] ");
> > + break;
> > + case mod:
> > + fn(data, NULL, "\n - [m] ");
> > + break;
> > + case no:
> > + fn(data, NULL, "\n - [ ] ");
> > + break;
> > + default:
> > + break;
>
> The default case is redundant. expr_calc_value() returns a
>
> typedef enum tristate {
> no, mod, yes
> } tristate;
>
> (There's a separate sym_get_string_value() that deals with "string
> values" of symbols.)
Is this variant better?
diff --git a/scripts/kconfig/expr.c b/scripts/kconfig/expr.c
index 0704bcdf4c78..4bf01269ce72 100644
--- a/scripts/kconfig/expr.c
+++ b/scripts/kconfig/expr.c
@@ -1185,7 +1185,24 @@ expr_print_newline(struct expr *e,
void *data,
int prevtoken)
{
- fn(data, NULL, "\n - ");
+ char buf[32];
+ const char *str;
+
+ switch (expr_calc_value(e)) {
+ case yes:
+ str = sym_get_string_value(&symbol_yes);
+ break;
+ case mod:
+ str = sym_get_string_value(&symbol_mod);
+ break;
+ case no:
+ /* Prefer '[ ]' to '[n]' for readability */
+ str = " ";
+ break;
+ }
+
+ sprintf(buf, "\n - [%s] ", str);
+ fn(data, NULL, buf);
expr_print(e, fn, data, prevtoken);
}
> > + }
> > expr_print(e, fn, data, prevtoken);
> > }
> >
> > --
> > 2.16.1
> >
>
> Looks good to me otherwise. I still like that earlier sorting idea,
> but this is a big improvement already.
To be honest, purely as a user, I would probably prefer something like
below:
Selected by [y/m]:
- EXPR_01
- EXPR_02
Selected by [n]:
- EXPR_03
- EXPR_04
But (as a developer) I feel it can only come at the price of increased
complexity of __expr_print() (at least, if not bigger than, [1]). What I
like about the current approach suggested by Masahiro is that it keeps
the impact low with still making a great improvement in readability.
If you think we should explore the second possibility of grouping the
active/inactive dependencies, I'll try to come up with some solution
in the next days.
[1] https://marc.info/?l=linux-kbuild&m=151777006005199&w=4
> Cheers,
> Ulf
Thanks for your review and comments!
Best regards,
Eugeniu.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
