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.vetr...@bugseng.com>
>>>>> ---
>>>>> 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
