Re: [PATCH 03/16] x86/monitor: mechanical renames

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
>> Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()-
>> don't have an 'arch_' prefix. Apply the same rule for monitor functions -
>> originally the only two monitor functions that had an 'arch_' prefix were
>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that
>> prefix because -they had a counterpart function in common code-, that being
>> monitor_domctl().
> This should actually be the other way around - ie adding the arch_
> prefix to vm_event functions that lack it.

Given that the majority of the arch-specific functions called from 
common-code don't have an 'arch_' prefix unless they have a common 
counterpart, I was guessing that was the rule. It made sense in my head 
since I saw in that the intention of avoiding naming conflicts (i.e you 
can't have monitor_domctl() both on the common-side and on the 
arch-side, so prepend 'arch_' to the latter). I noticed you guys also 
'skip' the prefix when sending patches, so that reinforced my assumption.

> Having the arch_ prefix is
> helpful to know that the function is dealing with the arch specific
> structs and not common.

Personally I don't see much use in 'knowing that the function is dealing 
with the arch structs' from the call-site and you can tell that from the 
implementation-site just by looking at the path of its source file. 
Also, the code is pretty much localized in the arch directory anyway so 
usually one wouldn't have to go back and forth between common and arch 
that often. What really bothers me though is always having to read 
'arch_' when spelling a function-name and also that it makes the 
function name longer without much benefit. Your suggestion of adding it 
to pretty much all functions that make up the interface to common just 
adds to that headache. :-D

> Similarly that's why we have the hvm_ prefix
> for functions in hvm/monitor.

'hvm_'  doesn't seem to me more special than 'monitor_', for instance, 
but maybe that's just me.

>> Let this also be the rule for future 'arch_' functions additions, and with this
>> patch remove the 'arch_' prefix from the monitor functions that don't have a
>> counterpart in common-code (all but those 2 aforementioned).
> Even if there are no common counter-parts to the function, the arch_
> prefix should remain, so I won't be able to ack this patch.
>
> Tamas

Having said the above, are you still of the same opinion?

Zuzu.

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