Change in coreboot[master]: soc/intel/*/acpi: Put braces around *else* branch

[Paul Menzel (Code Review)]

Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

soc/intel/*/acpi: Put braces around *else* branch

From `Documentation/coding_style.md`:
> This does not apply if only one branch of a conditional statement is a
> single statement; in the latter case use braces in both branches:

Change-Id: Idb4427f7ae9f7b99ba74f7e84aed6b8a901ca9fc
Signed-off-by: Paul Menzel 
---
M src/soc/intel/icelake/acpi.c
M src/soc/intel/jasperlake/acpi.c
M src/soc/intel/tigerlake/acpi.c
3 files changed, 6 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/42257/1

diff --git a/src/soc/intel/icelake/acpi.c b/src/soc/intel/icelake/acpi.c
index 796e770..12ce34c 100644
--- a/src/soc/intel/icelake/acpi.c
+++ b/src/soc/intel/icelake/acpi.c
@@ -198,8 +198,9 @@
    	if (CONFIG(EC_GOOGLE_CHROMEEC)) {
    		gnvs->chromeos.vbt2 = google_ec_running_ro() ?
    			ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
-		} else
+		} else {
    		gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO;
+		}
    }

/* Enable DPTF based on mainboard configuration */
diff --git a/src/soc/intel/jasperlake/acpi.c b/src/soc/intel/jasperlake/acpi.c
index 88ec5fe..250ff3b 100644
--- a/src/soc/intel/jasperlake/acpi.c
+++ b/src/soc/intel/jasperlake/acpi.c
@@ -292,8 +292,9 @@
    	if (CONFIG(EC_GOOGLE_CHROMEEC)) {
    		gnvs->chromeos.vbt2 = google_ec_running_ro() ?
    			ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
-		} else
+		} else {
    		gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO;
+		}
    }

/* Enable DPTF based on mainboard configuration */
diff --git a/src/soc/intel/tigerlake/acpi.c b/src/soc/intel/tigerlake/acpi.c
index 0991134..e965c48 100644
--- a/src/soc/intel/tigerlake/acpi.c
+++ b/src/soc/intel/tigerlake/acpi.c
@@ -292,8 +292,9 @@
    	if (CONFIG(EC_GOOGLE_CHROMEEC)) {
    		gnvs->chromeos.vbt2 = google_ec_running_ro() ?
    			ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
-		} else
+		} else {
    		gnvs->chromeos.vbt2 = ACTIVE_ECFW_RO;
+		}
    }

/* Enable DPTF based on mainboard configuration */

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2: Code-Review+2

Is it me, or are these files the same?

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
File src/soc/intel/jasperlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
PS2, Line 293: gnvs->chromeos.vbt2 = google_ec_running_ro() ?
             : 				ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
This is technically one statement. So why the brackets?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
File src/soc/intel/jasperlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
PS2, Line 293: gnvs->chromeos.vbt2 = google_ec_running_ro() ?
             : 				ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
> This is technically one statement. […]

Well, it uses multiple lines, so using braces makes the code clearer (even if only slightly).

But hey, we could also make everything a single statement!

gnvs->chromeos.vbt2 = !CONFIG(EC_GOOGLE_CHROMEEC) || google_ec_running_ro() ? ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;

(not that I would like it)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+2
>
> Is it me, or are these files the same?

Yes, there is a lot of code duplication.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:

(1 comment)

What to do? Add or remove braces?

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
File src/soc/intel/jasperlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
PS2, Line 293: gnvs->chromeos.vbt2 = google_ec_running_ro() ?
             : 				ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
> Well, it uses multiple lines, so using braces makes the code clearer (even if only slightly). […]

Raul, could catch, I didn’t see that. I’ll update the commit message.

PS: Patrick once proposed on the mailing list to require braces even around single line statements.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
File src/soc/intel/jasperlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/42257/2/src/soc/intel/jasperlake/ac... 
PS2, Line 293: gnvs->chromeos.vbt2 = google_ec_running_ro() ?
             : 				ACTIVE_ECFW_RO : ACTIVE_ECFW_RW;
> Raul, could catch, I didn’t see that. I’ll update the commit message. […]

Since coreboot is still no-braces for single statements, my preference is to remove them, and then indent l.294 an extra tabstop giving an extra visual cue that it's one statement broken over two lines.

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Paul Menzel.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Patch Set 2:

(1 comment)

Patchset:

PS2: 
abandon?

[Paul Menzel (Code Review)]

Paul Menzel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42257 )

Change subject: soc/intel/*/acpi: Put braces around *else* branch
......................................................................

Abandoned

Thank you for the review, and sorry for not going forward. It’s now obsolete and superseded by Kyösti’s GNVS work.