|SUCCESS| test: parenthesize assertion macro parameters

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163284

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-24

Commit: Unknown (from patch file series.patch)
Author: Weijun Pan <wpan3636 at gmail.com>
Subject: [PATCH] test: parenthesize assertion macro parameters

This patch adds parentheses around macro parameters in test assertion
macros to avoid unexpected evaluation due to operator precedence after
macro substitution.

> diff --git a/app/test/test.h b/app/test/test.h
> index 1f12fc5397..b1174e4973 100644
> --- a/app/test/test.h
> +++ b/app/test/test.h
> @@ -34,7 +34,7 @@
>  
>  /* Compare two buffers (length in bytes) */
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len,  msg, ...) do {	\
> -	if (memcmp(a, b, len)) {                                        \
> +	if (memcmp((a), (b), (len))) {                                        \

Does this code introduce trailing whitespace after the opening brace?
The original line appears to have trailing spaces, and this change
preserves them rather than removing them as required by the coding
standards.

> @@ -44,21 +44,21 @@
>  
>  /* Compare two buffers with offset (length and offset in bytes) */
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \
> -	const uint8_t *_a_with_off = (const uint8_t *)a + off;              \
> -	const uint8_t *_b_with_off = (const uint8_t *)b + off;              \
> -	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg);  \
> +	const uint8_t *_a_with_off = (const uint8_t *)(a) + (off);              \
> +	const uint8_t *_b_with_off = (const uint8_t *)(b) + (off);              \
> +	TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, (len), msg);  \

Does this code handle cases where the offset calculation could overflow?
With the parenthesization of (a) + (off), if 'a' is an expression like
'ptr + base', it becomes '(ptr + base) + (off)' which changes the
evaluation order. Is this the intended behavior?

> @@ -74,16 +74,16 @@
>  #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do {	\
>  	uint8_t _first_byte_a, _first_byte_b;                                 \
>  	uint8_t _first_byte_mask, _first_byte_bits;                           \
> -	uint32_t _len_without_first_byte = (off % 8) ?                       \
> -				len - (8 - (off % 8)) :                       \
> -				len;                                          \
> -	uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3);     \
> -	const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes;      \
> -	const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes;      \
> +	uint32_t _len_without_first_byte = ((off) % 8) ?                       \
> +				((len) - (8 - ((off) % 8))) :                       \
> +				(len);                                          \
> +	uint32_t _off_in_bytes = ((off) % 8) ? (((off) >> 3) + 1) : ((off) >> 3);     \

Does this line exceed the 79-character limit? The coding standards
require lines to be kept within 79 characters, and this line appears to
be longer.

Also, is the triple parenthesis (((off) >> 3) + 1) correct? While
((off) >> 3) + 1 would be sufficient since the shift operation already
has parentheses around 'off', the extra outer parenthesis may reduce
readability without adding value.

> diff --git a/lib/eal/include/rte_test.h b/lib/eal/include/rte_test.h
> index 62c8f165af..b73d4e75c2 100644
> --- a/lib/eal/include/rte_test.h
> +++ b/lib/eal/include/rte_test.h
> @@ -26,21 +26,21 @@
>  } while (0)
>  
>  #define RTE_TEST_ASSERT_EQUAL(a, b, msg, ...) \
> -	RTE_TEST_ASSERT(a == b, msg, ##__VA_ARGS__)
> +	RTE_TEST_ASSERT(((a) == (b)), msg, ##__VA_ARGS__)

Are double parentheses around the entire comparison expression necessary?
The pattern RTE_TEST_ASSERT((a) == (b), ...) would be sufficient and
more conventional. Does the outer set of parentheses serve a purpose, or
does it reduce readability?

The same question applies to all similar macros:
RTE_TEST_ASSERT_NOT_EQUAL, RTE_TEST_ASSERT_SUCCESS, RTE_TEST_ASSERT_FAIL,
RTE_TEST_ASSERT_NULL, and RTE_TEST_ASSERT_NOT_NULL.