[PATCH 1/2] kconfig: Print reverse dependencies on new line consistently

[PATCH 1/2] kconfig: Print reverse dependencies on new line consistently

[Eugeniu Rosca]

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 makes the implementation of
something like below more complicated:

Selected by:
- [m] F & G /* where (F & G) evaluates to '=m' */

Adding '[m]', '[y]' or '[ ]' to the left side of each reverse dependency
is subject of a different commit. The purpose of current commit is to
print the 'Selected by:' and 'Implied by:' 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 custom 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..0704bcdf4c78 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)
+				expr_print_newline(e, fn, data, E_OR);
+			else
+				fn(data, e->left.sym, e->left.sym->name);
 		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_newline(e, fn, data, E_OR);
+		} else {
+			expr_print(e->left.expr, fn, data, E_AND);
+			fn(data, NULL, " && ");
+			expr_print(e->right.expr, fn, data, E_AND);
+		}
 		break;
 	case E_LIST:
 		fn(data, e->right.sym, e->right.sym->name);
-- 
2.16.1

[PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca]

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 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@socionext.com>
Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 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;
+	}
 	expr_print(e, fn, data, prevtoken);
 }
 
-- 
2.16.1

Re: [PATCH 1/2] kconfig: Print reverse dependencies on new line consistently

[Ulf Magnusson]

On Tue, Feb 13, 2018 at 1:56 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 makes the implementation of
> something like below more complicated:
>
> Selected by:
> - [m] F & G /* where (F & G) evaluates to '=m' */
>
> Adding '[m]', '[y]' or '[ ]' to the left side of each reverse dependency
> is subject of a different commit. The purpose of current commit is to
> print the 'Selected by:' and 'Implied by:' 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 custom 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..0704bcdf4c78 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)
> +                               expr_print_newline(e, fn, data, E_OR);
> +                       else
> +                               fn(data, e->left.sym, e->left.sym->name);
>                 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_newline(e, fn, data, E_OR);
> +               } else {
> +                       expr_print(e->left.expr, fn, data, E_AND);
> +                       fn(data, NULL, " && ");
> +                       expr_print(e->right.expr, fn, data, E_AND);
> +               }
>                 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

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Ulf Magnusson]

On Tue, Feb 13, 2018 at 1:56 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 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@socionext.com>
> Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  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.)

> +       }
>         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.

Cheers,
Ulf

Re: [PATCH 1/2] kconfig: Print reverse dependencies on new line consistently

[Petr Vorel]

Hi Eugeniu,

> On Tue, Feb 13, 2018 at 1:56 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
I deliberately choose it :-). It didn't make much sense for me to add new line for just
one line. I thought someone wouldn't like it for the inconsistency :-).


Kind regards,
Petr

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca]

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@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 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@socionext.com>
> > Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >  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.

Re: [PATCH 1/2] kconfig: Print reverse dependencies on new line consistently

[Eugeniu Rosca]

Hi Petr,

On Tue, Feb 13, 2018 at 07:40:30AM +0100, Petr Vorel wrote:
> Hi Eugeniu,
> 
> > On Tue, Feb 13, 2018 at 1:56 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
> I deliberately choose it :-). It didn't make much sense for me to add new line for just
> one line. I thought someone wouldn't like it for the inconsistency :-).

I have to say I do like how `Selected by: F && G` looks on a single row.
However, when the expression is prefixed by its value (i.e. 
`Selected by: [m] F && G`), imho it doesn't look as crisp and clear
as we would like it to (feel free to disagree here). Fwiw, this patch
doesn't see itself as a fix, but rather prepares the ground for the next
commit implementing prefixing reverse dependencies by their expression
value.

> 
> Kind regards,
> Petr

Thanks!
Eugeniu.

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Petr Vorel]

Hi Eugeniu,

> 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
Sorting again, just with header? It looks more readable (although Masahiro's approach is
quite readable as well). If you chose this, I'd suggest not displaying title, when there
is empty list. i.e., don't show "Selected by [y/m]" in this:
Selected by [y/m]:
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


Kind regards,
Petr

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Petr Vorel]

Hi Eugeniu,

> 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 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]
Note, Masahiro suggested to have ':' after first expression [1]:
   Selected by:
    [y]: EEPROM_AT24 [=y] && I2C [=y] && SYSFS [=y]
    [ ]: NET_DSA_SMSC_LAN9303_I2C [=n] && NETDEVICES [=y] &&
HAVE_NET_DSA [=y] && NET_DSA [=n] && I2C [=y]
...

I think it's better, it's clearer that the expression is for whole line.


Kind regards,
Petr

[1]: https://marc.info/?l=linux-kbuild&m=151791725709825&w=4

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Ulf Magnusson]

On Wed, Feb 14, 2018 at 12:54 AM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> 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@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 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@socionext.com>
>> > Suggested-by: Ulf Magnusson <ulfalizer@gmail.com>
>> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>> > ---
>> >  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);
>  }
>

The older version was simpler. sym_get_string_value(&symbol_yes/no)
would never be anything other than "y"/"n" in practice. :)

Just removing the following two lines from the original version would
be best I think:

        default:
                break

>> > +       }
>> >         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

IMO, we could take this as is (after addressing other people's
comments), since it's already a big improvement, and then add sorting
later if we feel like it.

Don't have to do everything at once. :)

Think it should be pretty straightforward to add the kind of sorting
you had in mind at least, by changing PRINT_ALL_SELECTS into
PRINT_INACTIVE_SELECTS in [1], and tweaking some conditions.

>
>> Cheers,
>> Ulf
>
> Thanks for your review and comments!
>
> Best regards,
> Eugeniu.

Cheers,
Ulf

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca]

Hi Ulf,

On Wed, Feb 14, 2018 at 05:09:50AM +0100, Ulf Magnusson wrote:
> IMO, we could take this as is (after addressing other people's
> comments), since it's already a big improvement, and then add sorting
> later if we feel like it.
> 
> Don't have to do everything at once. :)

I agree that it's not possible to do everything perfect right from the
start. But since we are changing the user experience, I just thought
that we would first like to see how both implementations (prefixed
vs grouped tokens) look like, then make a decision (I'm still open
minded about it), then affect the users.

> 
> Think it should be pretty straightforward to add the kind of sorting
> you had in mind at least, by changing PRINT_ALL_SELECTS into
> PRINT_INACTIVE_SELECTS in [1], and tweaking some conditions.

Yep. Thanks to your support, I've pushed v3, where you've already
provided your review findings, which I'm going to fix soon :)

Thanks,
Eugeniu.

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca]

Hello Petr,

On Wed, Feb 14, 2018 at 01:41:54AM +0100, Petr Vorel wrote:
> Hi Eugeniu,
> 
> > 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
> Sorting again, just with header? It looks more readable (although Masahiro's approach is
> quite readable as well). If you chose this, I'd suggest not displaying title, when there
> is empty list. i.e., don't show "Selected by [y/m]" in this:
> Selected by [y/m]:
> Selected by [n]:
> - EXPR_03
> - EXPR_04

I took your preference into consideration in the v3 revision. Thanks!

Best regards,
Eugeniu.

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Ulf Magnusson]

On Sat, Feb 17, 2018 at 6:20 PM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> Hi Ulf,
>
> On Wed, Feb 14, 2018 at 05:09:50AM +0100, Ulf Magnusson wrote:
>> IMO, we could take this as is (after addressing other people's
>> comments), since it's already a big improvement, and then add sorting
>> later if we feel like it.
>>
>> Don't have to do everything at once. :)
>
> I agree that it's not possible to do everything perfect right from the
> start. But since we are changing the user experience, I just thought
> that we would first like to see how both implementations (prefixed
> vs grouped tokens) look like, then make a decision (I'm still open
> minded about it), then affect the users.

Isn't the latest patchset doing both? That seems like the best of both worlds.

And yeah, I just get some personal restlessness when good stuff floats
around for a long time without getting in. :)

Cheers,
Ulf

Re: [PATCH 2/2] kconfig: Print the value of each reverse dependency

[Eugeniu Rosca]

On Sat, Feb 17, 2018 at 06:31:48PM +0100, Ulf Magnusson wrote:
> On Sat, Feb 17, 2018 at 6:20 PM, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > Hi Ulf,
> >
> > On Wed, Feb 14, 2018 at 05:09:50AM +0100, Ulf Magnusson wrote:
> >> IMO, we could take this as is (after addressing other people's
> >> comments), since it's already a big improvement, and then add sorting
> >> later if we feel like it.
> >>
> >> Don't have to do everything at once. :)
> >
> > I agree that it's not possible to do everything perfect right from the
> > start. But since we are changing the user experience, I just thought
> > that we would first like to see how both implementations (prefixed
> > vs grouped tokens) look like, then make a decision (I'm still open
> > minded about it), then affect the users.
> 
> Isn't the latest patchset doing both? That seems like the best of both worlds.

Well, I call "prefixed tokens" solution [1] and "grouped tokens"
solution [2]. Assuming we share the same view, [1] has definitely less
of an impact on Kconfig (see [3]). That's the main argument to prefer it
over [2]. My personal choice is still [2]. However if it happens
that other reviewers prefer [1], I won't object on it.

[1] Selected by:
    - [y] EXPR_Y
    - [m] EXPR_M
    - [ ] EXPR_N

[2] Selected by [y]:
    - EXPR_Y
    Selected by [m]:
    - EXPR_M
    Selected by [n]:
    - EXPR_N

[3] https://marc.info/?l=linux-kbuild&m=151848343313335&w=4

> And yeah, I just get some personal restlessness when good stuff floats
> around for a long time without getting in. :)
> 
> Cheers,
> Ulf

Thanks,
Eugeniu.