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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 7/12/2016 9:10 AM, Corneliu ZUZU wrote:
> 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.
Can I get some extra feedback on this (Andrew, Stefano, Julien)? :-) 
(btw, is Jan on vacation?)
Thanks,
Corneliu.

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