Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.02.2024 17:40, Nicola Vetrini wrote:
> On 2024-02-29 17:25, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/kconfig.h
>>> +++ b/xen/include/xen/kconfig.h
>>> @@ -25,7 +25,7 @@
>>>  #define __ARG_PLACEHOLDER_1 0,
>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>  #define _config_enabled(value) 
>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> 1, 0)
>>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> (1), (0))
>>>  #define ___config_enabled(__ignored, val, ...) val
>>
>> In addition to what Andrew said, would you mind clarifying what exactly 
>> the
>> violation is here? I find it questionable that numeric literals need
>> parenthesizing; they don't normally need to, aynwhere.
>>
> 
> This one's a little special. I couldn't parenthesize val further down, 
> because then it would break the build:
> 
> In file included from ././include/xen/config.h:14,
>                   from <command-line>:
> ./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
> ‘)’ token
>     29 | #define ___config_enabled(__ignored, val, ...) (val)
>        |                                                    ^
> ./include/xen/kconfig.h:39:34: note: in expansion of macro 
> ‘___config_enabled’
>     39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
> static,)
>        |                                  ^~~~~~~~~~~~~~~~~
> ./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
>     38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
>        |                          ^~~~~~~~~~
> ./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
>     41 | #define STATIC_IF(option) static_if(option)
>        |                           ^~~~~~~~~
> common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
>    260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
> INVALID_MFN_INITIALIZER;
>        | ^~~~~~~~~
> make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1

So if we're not gong to deviate the construct, then this change needs to
come entirely on its own, with a really good description.

>>> --- a/xen/include/xen/list.h
>>> +++ b/xen/include/xen/list.h
>>> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
>>> list_head *list,
>>>   * @member: the name of the list_struct within the struct.
>>>   */
>>>  #define list_for_each_entry(pos, head, member)                        
>>>   \
>>> -    for (pos = list_entry((head)->next, typeof(*pos), member);        
>>>   \
>>> -         &pos->member != (head);                                      
>>>   \
>>> -         pos = list_entry(pos->member.next, typeof(*pos), member))
>>> +    for (pos = list_entry((head)->next, typeof(*(pos)), member);      
>>>     \
>>> +         &(pos)->member != (head);                                    
>>>   \
>>> +         pos = list_entry((pos)->member.next, typeof(*(pos)), 
>>> member))
>>
>> this ends up inconsistent, which I think isn't nice: Some uses of "pos"
>> are now parenthesized, while others aren't. Applies further down as 
>> well.
>>
> 
> Yeah, the inconsistency is due to the fact that a non-parenthesized 
> parameter as lhs is already deviated. To keep it consistent here I can 
> add parentheses, but then the deviation would be kind of pointless, 
> wouldn't it?

I don't think so: It would still be useful for cases where a macro
parameter is used solely as the lhs of some kind of assignment
operator. But yes, before making adjustments you will want to wait
for possible further comments, especially from e.g. Julien, who iirc
was primarily against this kind of parenthesization.

Jan