MISRA C Rule 5.3 violation - shadowing in mctelem.c

MISRA C Rule 5.3 violation - shadowing in mctelem.c

[Nicola Vetrini]

Hi all,

I was looking at the shadowing due to the struct identifier and the 
local variables "mctctl" in x86/cpu/mcheck/mctelem.c (see [1], the 
second report). This kind of shadowing seems very intentional, and the 
initial naive approach I devised was to simply rename the local 
variables.
This, however, results in build breakages, as sometimes the shadowed 
name seems to be used for accessing the global struct (unless I'm 
missing something), and as a result changing the name of the locals is 
not possible, at least not without further modifications to this file, 
which aren't obvious to me.

It would be really helpful if you could point me to either:
- avoid the shadowing in some way that does not occur to me at the 
moment;
- deviate this file, as many similar files in x86/cpu are already 
deviated.

What's your opinion on this?

Thanks,
   Nicola

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/latest/PROJECT.ecd;/by_service/MC3R1.R5.3.html

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: MISRA C Rule 5.3 violation - shadowing in mctelem.c

[Jan Beulich]

On 19.06.2024 15:23, Nicola Vetrini wrote:
> I was looking at the shadowing due to the struct identifier and the 
> local variables "mctctl" in x86/cpu/mcheck/mctelem.c (see [1], the 
> second report). This kind of shadowing seems very intentional, and the 
> initial naive approach I devised was to simply rename the local 
> variables.
> This, however, results in build breakages, as sometimes the shadowed 
> name seems to be used for accessing the global struct (unless I'm 
> missing something), and as a result changing the name of the locals is 
> not possible, at least not without further modifications to this file, 
> which aren't obvious to me.
> 
> It would be really helpful if you could point me to either:
> - avoid the shadowing in some way that does not occur to me at the 
> moment;

Could you please be more specific about the issues you encountered? I
hope you don't expect everyone reading this request of yours to (try to)
redo what you did. The only thing I could vaguely guess is that maybe
you went a little too far with the renaming. Plus, just from looking at
the grep output, did you try to simply move down the file scope variable?
It looks like all shadowing instances are ahead of any uses of the
variable (but I may easily be overlooking an important line contradicting
that pattern).

> - deviate this file, as many similar files in x86/cpu are already 
> deviated.

I question the presence of these in those files. They were apparently all
added when the files were introduced, and said commit - from Simone, acked
by Stefano - came with no justification at all.

Jan

Re: MISRA C Rule 5.3 violation - shadowing in mctelem.c

[Nicola Vetrini]

On 2024-06-19 15:42, Jan Beulich wrote:
> On 19.06.2024 15:23, Nicola Vetrini wrote:
>> I was looking at the shadowing due to the struct identifier and the
>> local variables "mctctl" in x86/cpu/mcheck/mctelem.c (see [1], the
>> second report). This kind of shadowing seems very intentional, and the
>> initial naive approach I devised was to simply rename the local
>> variables.
>> This, however, results in build breakages, as sometimes the shadowed
>> name seems to be used for accessing the global struct (unless I'm
>> missing something), and as a result changing the name of the locals is
>> not possible, at least not without further modifications to this file,
>> which aren't obvious to me.
>> 
>> It would be really helpful if you could point me to either:
>> - avoid the shadowing in some way that does not occur to me at the
>> moment;
> 
> Could you please be more specific about the issues you encountered? I
> hope you don't expect everyone reading this request of yours to (try 
> to)
> redo what you did. The only thing I could vaguely guess is that maybe
> you went a little too far with the renaming. Plus, just from looking at
> the grep output, did you try to simply move down the file scope 
> variable?
> It looks like all shadowing instances are ahead of any uses of the
> variable (but I may easily be overlooking an important line 
> contradicting
> that pattern).
> 

I think I found a way to refactor it without breaking the build, though 
I'm not sure whether it preserves the semantics of the code. I will send 
an RFC patch. Sorry for the noise.

>> - deviate this file, as many similar files in x86/cpu are already
>> deviated.
> 
> I question the presence of these in those files. They were apparently 
> all
> added when the files were introduced, and said commit - from Simone, 
> acked
> by Stefano - came with no justification at all.
> 
> Jan

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)