[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH v2 09/13] xen/public: fix violations of MISRA C:2012 Rule 7.2
On 06.07.23 10:43, Jan Beulich wrote:
On 06.07.2023 09:57, Juergen Gross wrote:
On 06.07.23 09:43, Jan Beulich wrote:
On 05.07.2023 17:33, Juergen Gross wrote:
On 05.07.23 17:26, Simone Ballarin wrote:
From: Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>
The xen sources contains violations of MISRA C:2012 Rule 7.2 whose
headline states:
"A 'u' or 'U' suffix shall be applied to all integer constants
that are represented in an unsigned type".
Add the 'U' suffix to integers literals with unsigned type and also to other
literals used in the same contexts or near violations, when their positive
nature is immediately clear. The latter changes are done for the sake of
uniformity.
Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
Signed-off-by: Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>
---
Changes in v2:
- minor change to commit title
- change commit message
- correct macros code style
---
xen/include/public/io/ring.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 025939278b..0cae4367be 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -36,11 +36,11 @@
typedef unsigned int RING_IDX;
/* Round a 32-bit unsigned constant down to the nearest power of two. */
-#define __RD2(_x) (((_x) & 0x00000002) ? 0x2 : ((_x) & 0x1))
-#define __RD4(_x) (((_x) & 0x0000000c) ? __RD2((_x)>>2)<<2 : __RD2(_x))
-#define __RD8(_x) (((_x) & 0x000000f0) ? __RD4((_x)>>4)<<4 : __RD4(_x))
-#define __RD16(_x) (((_x) & 0x0000ff00) ? __RD8((_x)>>8)<<8 : __RD8(_x))
-#define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x))
+#define __RD2(x) (((x) & 0x00000002U) ? 0x2 : ((x) & 0x1))
Shouldn't this be rather:
+#define __RD2(x) (((x) & 0x00000002U) ? 0x2U : ((x) & 0x1U))
I don't think it matters much (as the comment says, the input is expected
to be unsigned anyway), and I expect even the one U that was added here
was only added for consistency. The sole one that really matter is imo ...
+#define __RD4(x) (((x) & 0x0000000cU) ? __RD2((x) >> 2) << 2 : __RD2(x))
+#define __RD8(x) (((x) & 0x000000f0U) ? __RD4((x) >> 4) << 4 : __RD4(x))
+#define __RD16(x) (((x) & 0x0000ff00U) ? __RD8((x) >> 8) << 8 : __RD8(x))
+#define __RD32(x) (((x) & 0xffff0000U) ? __RD16((x) >> 16) << 16 : __RD16(x))
... this single one.
I agree that only the last one is really needed.
But for consistency reasons I'd expect all optional "U"s to be either dropped or
specified, instead of a mixture.
Funny you should say this. Shift counts also aren't allowed to be negative
... For this reason, the pattern I see here is to have U uniformly on the
lhs of the ?: operator, and nowhere else.
Yes, this is one way to look at it.
My view would be that (at least) the constants used for ANDing should have a
uniform U attribution.
In the end I'm fine with either way. I just wanted to point out a slight
inconsistency with the patch.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
