Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2023 09:08, Nicola Vetrini wrote:
> On 07/08/2023 11:10, Jan Beulich wrote:
>> On 07.08.2023 10:59, Nicola Vetrini wrote:
>>> On 07/08/2023 10:09, Jan Beulich wrote:
>>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>>> The variable declared in the header file
>>>>> 'xen/arch/x86/include/asm/e820.h'
>>>>> is shadowed by many function parameters, so it is renamed to avoid
>>>>> these
>>>>> violations.
>>>>>
>>>>> No functional changes.
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>>> ---
>>>>> This patch is similar to other renames done on previous patches, and
>>>>> the
>>>>> preferred strategy there was to rename the global variable. This one
>>>>> has more occurrences that are spread in various files, but
>>>>> the general pattern is the same.
>>>>
>>>> Still I think it would be better done the other way around, and 
>>>> perhaps
>>>> in
>>>> more than a single patch. It looks like "many == 3", i.e.
>>>> - e820_add_range(), which is only ever called with "e820" as its
>>>> argument,
>>>>   and hence the parameter could be dropped,
> 
> I see another downside with this approach (I should have spotted this 
> sooner):
> Since e820_add_range and the other functions expected e820 as a pointer, 
> they are
> written like this:
> 
> for ( i = 0; i < e820->nr_map; ++i )
>      {
>          uint64_t rs = e820->map[i].addr;
>          uint64_t re = rs + e820->map[i].size;
> 
>          if ( rs == e && e820->map[i].type == type )
>          {
>              e820->map[i].addr = s;
>              return 1;
>          }
> ...
> 
> Dropping the parameter would either mean
> 1. Use a local parameter that stores the address of e820, which kind of
>     nullifies the purpose of dropping the parameter imho;

This isn't an unusual thing to do; it is only the name being short which
may make it look "unnecessary" here. But especially if the local variable
was made of type struct e820entry * (and updated in the for()) I think
this could be useful overall.

> 2. Rewrite it so that it operates on a "struct e820map", which would 
> mean
>     substantial churn;
> 3. Make the global a pointer, which is reminiscent of (1)
> 
> All in all, I do like the global renaming approach more, because it 
> lessens
> the amount of code that needs to change to accomodate a case of 
> shadowing.

Well, to go that route you need to come up with a suitable new name (no
prior proposal was really meeting that requirement) and you'd need to
convince at least one of the x86 maintainers.

Jan