Change in ...coreboot[master]: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)

[Julius Werner (Code Review)]

Hello Aaron Durbin, Patrick Georgi,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/31773

to review the following change.

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)

The IS_ENABLED() macro is pretty long and unwieldy for something so
widely used, and often forces line breaks just for checking two Kconfigs
in a row. Let's replace it with something that takes up less space to
make our code more readable. From now on,

if (IS_ENABLED(CONFIG_XXX))
 #if IS_ENABLED(CONFIG_XXX)

shall become

if (CONFIG(XXX))
 #if CONFIG(XXX)

Change-Id: I2468427b569b974303084574125a9e1d9f6db596
Signed-off-by: Julius Werner 
---
M payloads/libpayload/include/kconfig.h
M src/include/kconfig.h
2 files changed, 6 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/31773/1

diff --git a/payloads/libpayload/include/kconfig.h b/payloads/libpayload/include/kconfig.h
index adb3403..3a44529 100644
--- a/payloads/libpayload/include/kconfig.h
+++ b/payloads/libpayload/include/kconfig.h
@@ -17,5 +17,7 @@
 #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0, 0)
 #define ___config_enabled(__ignored, val, ...) val

-#define IS_ENABLED(option) config_enabled(option)
+#define IS_ENABLED(option) config_enabled(option)	/* deprecated */
+#define CONFIG(option) config_enabled(CONFIG_#option)
+
 #endif
diff --git a/src/include/kconfig.h b/src/include/kconfig.h
index 73106e9..0478548 100644
--- a/src/include/kconfig.h
+++ b/src/include/kconfig.h
@@ -17,5 +17,7 @@
 #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0, 0)
 #define ___config_enabled(__ignored, val, ...) val

-#define IS_ENABLED(option) config_enabled(option)
+#define IS_ENABLED(option) config_enabled(option)	/* deprecated */
+#define CONFIG(option) config_enabled(CONFIG_##option)
+
 #endif

[Julius Werner (Code Review)]

Hello Aaron Durbin, Martin Roth, Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31773

to look at the new patch set (#2).

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)

The IS_ENABLED() macro is pretty long and unwieldy for something so
widely used, and often forces line breaks just for checking two Kconfigs
in a row. Let's replace it with something that takes up less space to
make our code more readable. From now on,

if (IS_ENABLED(CONFIG_XXX))
 #if IS_ENABLED(CONFIG_XXX)

shall become

if (CONFIG(XXX))
 #if CONFIG(XXX)

Change-Id: I2468427b569b974303084574125a9e1d9f6db596
Signed-off-by: Julius Werner 
---
M payloads/libpayload/include/kconfig.h
M src/include/kconfig.h
2 files changed, 6 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/31773/2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Patch Set 2: Code-Review+1

I'm ok with the change in concept.  I'll note that because it touches so many files it's going to be a *LITTLE* disruptive to people working on private branches. That's the only issue I see.

The command to create the change in the commit message of the next patch does make things somewhat easier so that people can create their own patches to update private branches.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> I'm ok with the change in concept.  I'll note that because it touches so many files it's going to be a *LITTLE* disruptive to people working on private branches. That's the only issue I see.

Since IS_ENABLED is still supported, it's not really disruptive. With the other changes it's now a lint warning. After the next release (late April I guess) we can make it a lint error. That way only branches older than two months will run into it.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Patch Set 2: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Patch Set 2:
> Since IS_ENABLED is still supported, it's not really disruptive. With the
> other changes it's now a lint warning. After the next release (late April
> I guess) we can make it a lint error. That way only branches older than
> two months will run into it.

I'm talking about merging patches from master into other branches.  The change will have to be applied to any branch that's still being patched from coreboot or new patches that include the new macro won't apply cleanly.

The patch to change the macros will have to be re-created for each remote branch using the command in the next commit because that patch won't apply cleanly.

Once the change is applied to a branch, any patch from before the change that includes IS_ENABLED will no longer apply cleanly.

As I said, it's a LITTLE disruptive just because it's so broad.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Patch Set 2:
> Once the change is applied to a branch, any patch from before the change that includes IS_ENABLED will no longer apply cleanly.

Ack, but I don't think there's anything more I can do to make this easier, and I don't think it's a sufficient justification to not make sweeping changes if we otherwise want them (because then we never could). In-flight patches will just have to keep up with whatever lands upstream, that's how it has always been for any patch.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31773 )

Change subject: Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)
......................................................................

Add new CONFIG(XXX) macro to replace IS_ENABLED(CONFIG_XXX)

The IS_ENABLED() macro is pretty long and unwieldy for something so
widely used, and often forces line breaks just for checking two Kconfigs
in a row. Let's replace it with something that takes up less space to
make our code more readable. From now on,

if (IS_ENABLED(CONFIG_XXX))
 #if IS_ENABLED(CONFIG_XXX)

shall become

if (CONFIG(XXX))
 #if CONFIG(XXX)

Change-Id: I2468427b569b974303084574125a9e1d9f6db596
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31773
Tested-by: build bot (Jenkins) 
Reviewed-by: Martin Roth 
Reviewed-by: Patrick Georgi 
---
M payloads/libpayload/include/kconfig.h
M src/include/kconfig.h
2 files changed, 6 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Martin Roth: Looks good to me, but someone else must approve

diff --git a/payloads/libpayload/include/kconfig.h b/payloads/libpayload/include/kconfig.h
index adb3403..9cce6ea 100644
--- a/payloads/libpayload/include/kconfig.h
+++ b/payloads/libpayload/include/kconfig.h
@@ -17,5 +17,7 @@
 #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0, 0)
 #define ___config_enabled(__ignored, val, ...) val

-#define IS_ENABLED(option) config_enabled(option)
+#define IS_ENABLED(option) config_enabled(option)	/* deprecated */
+#define CONFIG(option) config_enabled(CONFIG_##option)
+
 #endif
diff --git a/src/include/kconfig.h b/src/include/kconfig.h
index 73106e9..0478548 100644
--- a/src/include/kconfig.h
+++ b/src/include/kconfig.h
@@ -17,5 +17,7 @@
 #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0, 0)
 #define ___config_enabled(__ignored, val, ...) val

-#define IS_ENABLED(option) config_enabled(option)
+#define IS_ENABLED(option) config_enabled(option)	/* deprecated */
+#define CONFIG(option) config_enabled(CONFIG_##option)
+
 #endif