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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>
>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
>> 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?

Yes, I am. While it's not a hard rule to always apply these prefix, it
does make sense to have them so I don't see benefit in removing the
existing prefixes. Adding arch_ prefix to the ones that don't already
have one is optional, I was just pointing out that if you really feel
like standardizing the naming convention, that's where I would like
things to move towards to.

Tamas

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