Re: [Xen-devel] [PATCH] x86/HVM: fold hypercall tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.16 at 15:35, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/02/16 08:52, Jan Beulich wrote:
>>>>> On 15.02.16 at 09:26, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 15/02/2016 07:42, Jan Beulich wrote:
>>>> @@ -5395,7 +5398,7 @@ int hvm_do_hypercall(struct cpu_user_reg
>>>>          }
>>>>  #endif
>>>>  
>>>> -        regs->_eax = hvm_hypercall32_table[eax](ebx, ecx, edx, esi, edi, 
>>>> ebp);
>>>> +        regs->_eax = hypercall_table[eax].compat(ebx, ecx, edx, esi, edi, 
>>> ebp);
>>>>  
>>> I know its in a different translation unit, but we already have a
>>> hypercall_table and it is a global symbol.  Please could we retain the
>>> hvm_ prefix here.
>> I'm aware of that and removed the prefix knowingly. I see no
>> reason why it needs to be there.
> 
> Redundant prefixes like this are not for the benefit of the compiler. 
> They are for the benefit of humans reading the code.
> 
> You, as an individual who is very familiar with the codebase, might have
> no problem identifying which actual symbol is intended.
> 
> Being open source, the intended audience is the average C programmer who
> can make alterations, but isn't completely familiar with the codebase,
> and likely be navigating the source code with tools such as
> grep/cscope/ctags/other.

Any of these should easily point out that this symbol is local
to this translation unit. Hence to me your argumentation makes
no sense.

>>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Please clarify whether the R-b stands nevertheless, or whether
>> we need to have a longer debate first.
> 
> My R-b does not stand.

Okay, to avoid this becoming an endless discussion, I'll make the
change. The only _rational_ argument I would accept here is that
hypercall_table[], while not declared in any header, currently is
global, and hence there would be a conflict the moment it would
get declared in some header.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel