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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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;
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.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)