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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
> 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.
Well, for one the benefit would be not confusing developers by creating 
inconsistencies: what's the rule here, i.e. why isn't a function such as 
alloc_domain_struct prefixed w/ 'arch_' but arch_domain_create is? The 
reason seems to be the latter having a common counterpart while the 
former not, at least that's what I see being done all over the 
code-base. Also, I've done this before and you seemed to agree: 
https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html 
(Q1). You also suggested creating arch-specific functions without the 
prefix: 
https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html . 
Why the sudden change of heart?
2ndly and obviously, removing the prefixes would make function names 
shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and then 
read "vm_event_vcpu_unpause").
3rd reason is that adding the prefix to -all- arch-specific functions 
called from common would mean having a lot new functions with the 
prefix. I'd read the prefix over and over again and at some point I'd 
get annoyed and say "ok, ok, it's arch_, I get it; why use this prefix 
so much again?".
4th reason is that the advantage of telling that the function accesses 
arch structures is much too little considering that idk, >50% of the 
codebase is arch-specific, so it doesn't provide much information, this 
categorization is too broad to deserve a special prefix. Whereas using 
the prefix only for functions that do have a common counterpart gives 
you the extra information that the 'operation' is only -partly- 
arch-specific, i.e. to see the whole picture, look @ the common-side 
implementation. Keep in mind that we'd also be -losing that information- 
if we were to apply the 'go with arch_ for all' rule.. (this could be a 
5th reason)
> 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
I don't think I'd say this patch "standardizes the naming convention" 
but rather "fixes code that doesn't conform to the -already existing- 
standard naming convention". Above stated reasons explain why I'd rather 
-not- have this standard go in the direction of adding 'arch_' before a 
lot of functions.
Finally, I feel like asking for feedback as I don't like to insist when 
the majority agrees to disagree. Jan & others, what's the rule here and 
what's your view on this? :-)
Thanks,
Zuzu C.

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