Change in coreboot[master]: mb/google/parrot: Put trailing statement in the next line

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

mb/google/parrot: Put trailing statement in the next line

Fixes a linter error.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 2 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/49201/1

diff --git a/src/mainboard/google/parrot/smihandler.c b/src/mainboard/google/parrot/smihandler.c
index 9d96473..c27e15e 100644
--- a/src/mainboard/google/parrot/smihandler.c
+++ b/src/mainboard/google/parrot/smihandler.c
@@ -39,7 +39,8 @@
    printk(BIOS_DEBUG, "mainboard_smi_gpi: %x\n", gpi_sts);
    if (gpi_sts & (1 << EC_SMI_GPI)) {
    	/* Process all pending events from EC */
-		while (mainboard_smi_ec() != EC_NO_EVENT);
+		while (mainboard_smi_ec() != EC_NO_EVENT)
+			;
    } else if (gpi_sts & (1 << EC_LID_GPI)) {
    	printk(BIOS_DEBUG, "LID CLOSED, SHUTDOWN\n");

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1: Code-Review+2

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1:

(1 comment)

Patchset:

PS1: 
Or fix the linter instead?

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1:

(1 comment)

Patchset:

PS1: 
According to our coding style [1], this time the linter is correct:
> Don’t put multiple statements on a single line unless you have something to hide:
>  if (condition) do_this;
>    do_something_everytime;

[1]: https://doc.coreboot.org/coding_style.html#indentation

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49201/comment/fc15cd2e_268fefc3 
PS1, Line 9: Fixes a linter error.
You can also mention this is to comply with the coreboot coding style guidelines, and also improves readability (these busy-waiting while loops can look a lot like an ordinary function call when the semicolon is placed on the same line)

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1:

(1 comment)

Patchset:

PS1:
> According to our coding style [1], this time the linter is correct: […]

But is hiding an empty statement hiding something?

Anyway, IMO, the only good solution is to move the side-effect
expression out of the condition. One would need to add some
state variable in this case, though.

My usual argument against the semicolon on an empty line is
my fear that at some point somebody will grep for useless
semicolons and reviewers might miss to correct some of the
false positives.

Or how about this:

while (side_effect()) {
            /* do nothing */
    }

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

Patchset:

PS1:
> But is hiding an empty statement hiding something? […]

Good point. To me, the while with in-line semicolon construct is very easy to mistake for a regular function call. I agree that the lone semicolon can be removed and wreak all kinds of havoc, though.

Another approach that would work is this:

while (1) {
         if (exit_condition_true())
                 break;
 }

We could also use a helper macro:

#define wait_until(cond) do { } while (!(cond))

Wait... This compiles!

do; while (!exit_condition_true());

[Felix Singer (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons.
 Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 1:

(1 comment)

Patchset:

PS1:
> To me, the while with in-line semicolon construct is very easy to mistake for a regular function call.

Same, it's kind of irritating.

`do; while (!exit_condition_true());`

It not only compiles, linter seems to be happy with that :) So should I change it to this?

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Put trailing statement in the next line
......................................................................

Patch Set 2:

(1 comment)

Patchset:

PS1:
> > To me, the while with in-line semicolon construct is very easy to mistake for a regular function c […]

I'm ok with it. Though, I feel like it might get more acceptance with {}:

do {} while (!exit_condition_true());

There would probably be less doubts if it compiles :D
But, whatever makes the linter happy and doesn't have a lone ; on a line is ok
for me.

Change in coreboot[master]: mb/google/parrot: Move while loop to do-while

[Felix Singer (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons.
Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: mb/google/parrot: Move while loop to do-while
......................................................................

mb/google/parrot: Move while loop to do-while

Fixes linter error complaining about trailing semicolon.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/49201/3

Change in coreboot[master]: mb/google/parrot: Move while loop to do-while

[Felix Singer (Code Review)]

Attention is currently required from: Nico Huber, Angel Pons.
 Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Move while loop to do-while
......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS1:
> I'm ok with it. Though, I feel like it might get more acceptance with {}: […]

Done

Change in coreboot[master]: mb/google/parrot: Move while loop to do-while

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Move while loop to do-while
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49201/comment/535b4e45_8ee0e676 
PS3, Line 9: Fixes linter error complaining about trailing semicolon.
... and makes it clear that the loop body is empty on purpose?

Change in coreboot[master]: mb/google/parrot: Move while loop to do-while

[Angel Pons (Code Review)]

Attention is currently required from:  Felix Singer.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Move while loop to do-while
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49201/comment/436fe551_145c989d 
PS3, Line 7: Move
nit: Convert

Change in coreboot[master]: mb/google/parrot: Move while loop to do-while

[Felix Singer (Code Review)]

Attention is currently required from:  Felix Singer.
Hello build bot (Jenkins), Nico Huber, Angel Pons,

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

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

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

Change subject: mb/google/parrot: Move while loop to do-while
......................................................................

mb/google/parrot: Move while loop to do-while

Fixes linter error complaining about trailing semicolon.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/49201/5

Change in coreboot[master]: mb/google/parrot: Replace while-loop to do-while

[Felix Singer (Code Review)]

Attention is currently required from:  Felix Singer.
Hello build bot (Jenkins), Nico Huber, Angel Pons,

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

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

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

Change subject: mb/google/parrot: Replace while-loop to do-while
......................................................................

mb/google/parrot: Replace while-loop to do-while

Fixes linter error complaining about trailing semicolon.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/49201/6

Change in coreboot[master]: mb/google/parrot: Replace while-loop with do-while

[Felix Singer (Code Review)]

Attention is currently required from:  Felix Singer.
Hello build bot (Jenkins), Nico Huber, Angel Pons,

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

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

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

Change subject: mb/google/parrot: Replace while-loop with do-while
......................................................................

mb/google/parrot: Replace while-loop with do-while

Fixes linter error complaining about trailing semicolon.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/49201/7

Change in coreboot[master]: mb/google/parrot: Replace while-loop with do-while

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49201 )

Change subject: mb/google/parrot: Replace while-loop with do-while
......................................................................

mb/google/parrot: Replace while-loop with do-while

Fixes linter error complaining about trailing semicolon.

Change-Id: I3f74f25cb2e3edcdd509abd86d80098241c05741
Signed-off-by: Felix Singer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49201
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Angel Pons 
---
M src/mainboard/google/parrot/smihandler.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/parrot/smihandler.c b/src/mainboard/google/parrot/smihandler.c
index 0a4f249..e0192ce 100644
--- a/src/mainboard/google/parrot/smihandler.c
+++ b/src/mainboard/google/parrot/smihandler.c
@@ -39,7 +39,7 @@
    printk(BIOS_DEBUG, "%s: %x\n", __func__, gpi_sts);
    if (gpi_sts & (1 << EC_SMI_GPI)) {
    	/* Process all pending events from EC */
-		while (mainboard_smi_ec() != EC_NO_EVENT);
+		do {} while (mainboard_smi_ec() != EC_NO_EVENT);
    } else if (gpi_sts & (1 << EC_LID_GPI)) {
    	printk(BIOS_DEBUG, "LID CLOSED, SHUTDOWN\n");