Hi Julien,
Hi Julien,
On 7/29/22 18:15, Julien Grall wrote:
Hi Xenia,
On 29/07/2022 15:03, Xenia Ragiadakou wrote:
On 7/29/22 16:41, Bertrand Marquis wrote:
Hi Xenia,
On 29 Jul 2022, at 07:31, Xenia Ragiadakou <[email protected]>
wrote:
Hi Jan,
On 7/29/22 09:26, Jan Beulich wrote:
On 28.07.2022 18:21, Xenia Ragiadakou wrote:
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -63,7 +63,7 @@ static void do_idle(void)
rcu_idle_exit(cpu);
}
-void idle_loop(void)
+static void idle_loop(void)
While you're adding "noreturn" below, shouldn't this one be marked so
as well? Preferably with the addition:
Reviewed-by: Jan Beulich <[email protected]>
Yes, but I was not sure if this should go in this patch or in a
separate one.
As you modify the function to make it static, I think it is ok to
also add the noreturn in the same patch.
With that done, you can add my:
Reviewed-by: Bertrand Marquis <[email protected]>
Cheers
Bertrand
I consider this change unrelated to the patch. I think it is a bad
practice to squash unrelated changes in a single patch. Also, I do not
think it is unfair to be obliged to make it in order for the patch to
be accepted.
I could have taken the opportunity to fix this in the same patch but I
decided to not take it.
In general, I don't like having multiple changes within a patch.
However, here this is a consistency problem. You are modifying the 3
prototypes (well one is technically a declaration) and it reads odd that
2 are using noreturn but not the other one.
The patch adds the 2 function declarations, it does not modify them.
Since they do not return, they are declared noreturn.
If the function idle_loop was not declared noreturn, although it should
have been, for me is a completely different issue.
I would actually argue that if this patch goes in like that, then the
commit message ought to explain why there is a lack of consistency.
I do not agree with you saying that the patch introduced this
inconsistency. The function idle_loop should have been declared noreturn
in the first place. If you would like to fix this in the current patch,
it is up to you.
Anyway, I agree with Bertrand that it would be preferable to add
noreturn to the declaration of idle_loop() in this patch.
To avoid a round trip, I would be OK to handle on commit.
Cheers,
--
Xenia
