[PATCH v2 0/3] xen/cpu: Minor coding style fixes

[PATCH v2 0/3] xen/cpu: Minor coding style fixes

[Xenia Ragiadakou]

Xenia Ragiadakou (3):
  xen/cpu: Fix MISRA C 2012 Rule 20.7 violation
  xen/cpu: Add missing white space around arithmetic operators
  xen/cpu: Undefine MASK_DECLARE_ macros after their usage

 xen/common/cpu.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] xen/cpu: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

In MASK_DECLARE_ macros, the macro parameter 'x' is used as expression and
therefore it is good to be enclosed in parentheses to prevent against
unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v2:
- fix MISRA C 2012 Rule 20.7 violation in all MASK_DECLARE_ macros

 xen/common/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index b0b63cdb36..feb2a6634e 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -25,10 +25,10 @@ const cpumask_t cpumask_all = {
  */
 
 /* cpu_bit_bitmap[0] is empty - so we can back into it */
-#define MASK_DECLARE_1(x) [x+1][0] = 1UL << (x)
-#define MASK_DECLARE_2(x) MASK_DECLARE_1(x), MASK_DECLARE_1(x+1)
-#define MASK_DECLARE_4(x) MASK_DECLARE_2(x), MASK_DECLARE_2(x+2)
-#define MASK_DECLARE_8(x) MASK_DECLARE_4(x), MASK_DECLARE_4(x+4)
+#define MASK_DECLARE_1(x) [(x)+1][0] = 1UL << (x)
+#define MASK_DECLARE_2(x) MASK_DECLARE_1(x), MASK_DECLARE_1((x)+1)
+#define MASK_DECLARE_4(x) MASK_DECLARE_2(x), MASK_DECLARE_2((x)+2)
+#define MASK_DECLARE_8(x) MASK_DECLARE_4(x), MASK_DECLARE_4((x)+4)
 
 const unsigned long cpu_bit_bitmap[BITS_PER_LONG+1][BITS_TO_LONGS(NR_CPUS)] = {
 
-- 
2.34.1

[PATCH v2 2/3] xen/cpu: Add missing white space around arithmetic operators

[Xenia Ragiadakou]

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v2:
- new patch

 xen/common/cpu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index feb2a6634e..c48a1cabd2 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -25,12 +25,12 @@ const cpumask_t cpumask_all = {
  */
 
 /* cpu_bit_bitmap[0] is empty - so we can back into it */
-#define MASK_DECLARE_1(x) [(x)+1][0] = 1UL << (x)
-#define MASK_DECLARE_2(x) MASK_DECLARE_1(x), MASK_DECLARE_1((x)+1)
-#define MASK_DECLARE_4(x) MASK_DECLARE_2(x), MASK_DECLARE_2((x)+2)
-#define MASK_DECLARE_8(x) MASK_DECLARE_4(x), MASK_DECLARE_4((x)+4)
+#define MASK_DECLARE_1(x) [(x) + 1][0] = 1UL << (x)
+#define MASK_DECLARE_2(x) MASK_DECLARE_1(x), MASK_DECLARE_1((x) + 1)
+#define MASK_DECLARE_4(x) MASK_DECLARE_2(x), MASK_DECLARE_2((x) + 2)
+#define MASK_DECLARE_8(x) MASK_DECLARE_4(x), MASK_DECLARE_4((x) + 4)
 
-const unsigned long cpu_bit_bitmap[BITS_PER_LONG+1][BITS_TO_LONGS(NR_CPUS)] = {
+const unsigned long cpu_bit_bitmap[BITS_PER_LONG + 1][BITS_TO_LONGS(NR_CPUS)] = {
 
     MASK_DECLARE_8(0),  MASK_DECLARE_8(8),
     MASK_DECLARE_8(16), MASK_DECLARE_8(24),
-- 
2.34.1

[PATCH v2 3/3] xen/cpu: Undefine MASK_DECLARE_ macros after their usage

[Xenia Ragiadakou]

MASK_DECLARE_ macros have only a limited scope. Remove their definitions
immediately after their usage.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v2:
- new patch

 xen/common/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index c48a1cabd2..4a048caa49 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -40,6 +40,11 @@ const unsigned long cpu_bit_bitmap[BITS_PER_LONG + 1][BITS_TO_LONGS(NR_CPUS)] =
 #endif
 };
 
+#undef MASK_DECLARE_8
+#undef MASK_DECLARE_4
+#undef MASK_DECLARE_2
+#undef MASK_DECLARE_1
+
 static DEFINE_RWLOCK(cpu_add_remove_lock);
 
 bool get_cpu_maps(void)
-- 
2.34.1

Re: [PATCH v2 0/3] xen/cpu: Minor coding style fixes

[Jan Beulich]

On 05.08.2022 14:44, Xenia Ragiadakou wrote:
> Xenia Ragiadakou (3):
>   xen/cpu: Fix MISRA C 2012 Rule 20.7 violation
>   xen/cpu: Add missing white space around arithmetic operators
>   xen/cpu: Undefine MASK_DECLARE_ macros after their usage
> 
>  xen/common/cpu.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Acked-by: Jan Beulich <jbeulich@suse.com>

However,
- I don't see why patches 1 and 2 needed splitting, when patch 1 already
  touches all those lines. It is the usual thing for us to make cosmetic
  adjustments when touching a line anyway.
- Patch 3, while fine to be separate, wants a Requested-by: or
  Suggested-by: me (which I guess can be taken care of while committing).

Jan

Re: [PATCH v2 0/3] xen/cpu: Minor coding style fixes

[Bertrand Marquis]

Hi Xenia,

> On 5 Aug 2022, at 13:44, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> Xenia Ragiadakou (3):
>  xen/cpu: Fix MISRA C 2012 Rule 20.7 violation
>  xen/cpu: Add missing white space around arithmetic operators
>  xen/cpu: Undefine MASK_DECLARE_ macros after their usage
> 
> xen/common/cpu.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
> 

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

But I agree with Jan that some patches could have been
squashed but I do not think we need to have a v3 here.

Cheers
Bertrand

> -- 
> 2.34.1
> 
> 

Re: [PATCH v2 0/3] xen/cpu: Minor coding style fixes

[Xenia Ragiadakou]

Hi Jan,

On 8/5/22 15:50, Jan Beulich wrote:
> On 05.08.2022 14:44, Xenia Ragiadakou wrote:
>> Xenia Ragiadakou (3):
>>    xen/cpu: Fix MISRA C 2012 Rule 20.7 violation
>>    xen/cpu: Add missing white space around arithmetic operators
>>    xen/cpu: Undefine MASK_DECLARE_ macros after their usage
>>
>>   xen/common/cpu.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> However,
> - I don't see why patches 1 and 2 needed splitting, when patch 1 already
>    touches all those lines. It is the usual thing for us to make cosmetic
>    adjustments when touching a line anyway.

In my opinion, the initial patch that added the code should not have 
been accepted in first place without the white spaces around '+'.
But maybe coding style rules came later.
Nevertheless, I continue to consider it unfair to rely on and request 
from new unrelated patches to fix those issues.

> - Patch 3, while fine to be separate, wants a Requested-by: or
>    Suggested-by: me (which I guess can be taken care of while committing).
> 
> Jan

-- 
Xenia

Re: [PATCH v2 0/3] xen/cpu: Minor coding style fixes

[Jan Beulich]

On 05.08.2022 15:05, Xenia Ragiadakou wrote:
> On 8/5/22 15:50, Jan Beulich wrote:
>> On 05.08.2022 14:44, Xenia Ragiadakou wrote:
>>> Xenia Ragiadakou (3):
>>>    xen/cpu: Fix MISRA C 2012 Rule 20.7 violation
>>>    xen/cpu: Add missing white space around arithmetic operators
>>>    xen/cpu: Undefine MASK_DECLARE_ macros after their usage
>>>
>>>   xen/common/cpu.c | 15 ++++++++++-----
>>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> However,
>> - I don't see why patches 1 and 2 needed splitting, when patch 1 already
>>    touches all those lines. It is the usual thing for us to make cosmetic
>>    adjustments when touching a line anyway.
> 
> In my opinion, the initial patch that added the code should not have 
> been accepted in first place without the white spaces around '+'.
> But maybe coding style rules came later.

Iirc the goal was to take it unaltered from Linux.

> Nevertheless, I continue to consider it unfair to rely on and request 
> from new unrelated patches to fix those issues.

I can certainly see where you're coming from, but please also try to take
maintainer perspective: By doing trivial and obvious adjustments at the
same time when touching code anyway, there's one less patch to look at
(and later to apply). And please also consider how "git blame" or alike
works: By touching the same line(s) twice in close succession, finding
the "real" change is made needlessly harder. (Putting the cosmetic change
first is generally disliked because it makes backporting harder, in case
such is wanted / needed.)

I'm sorry if you take such requests as unfair - they aren't meant to be.
They're merely meant to make things as easy as possible for everyone
(which may only be on average, yes).

Jan