Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/17/2016 10:06 AM, Jan Beulich wrote:
> > > > On 16.06.16 at 21:19, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > On 6/16/2016 5:24 PM, Jan Beulich wrote:
> > > > > > On 16.06.16 at 16:06, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > > > --- a/xen/arch/x86/hvm/event.c
> > > > +++ b/xen/arch/x86/hvm/event.c
> > > > @@ -23,6 +23,7 @@
> > > >    
> > > >    #include <xen/vm_event.h>
> > > >    #include <asm/hvm/event.h>
> > > > +#include <asm/paging.h>
> > > >    #include <asm/monitor.h>
> > > >    #include <asm/vm_event.h>
> > > >    #include <public/vm_event.h>
> > > > diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> > > > index d950a7c..b30857a 100644
> > > > --- a/xen/common/monitor.c
> > > > +++ b/xen/common/monitor.c
> > > > @@ -22,7 +22,6 @@
> > > >    #include <xen/monitor.h>
> > > >    #include <xen/sched.h>
> > > >    #include <xsm/xsm.h>
> > > > -#include <public/domctl.h>
> > > >    #include <asm/monitor.h>
> > > >    #include <asm/vm_event.h>
> > > These two adjustments clearly don't fit title / description. I certainly
> > > don't mind unnecessary inclusions to be dropped, but the addition of
> > > one clearly needs explanation (after all thing build fine without it).
> > Sorry, that was done out of reflex, should have stated the reasoning.
> > Generally, if:
> > - event.c includes event.h
> > - event.c needs paging.h
> > - event.h -doesn't need- paging.h
> > then I prefer to include paging.h in event.c, not in event.h (include
> > strictly -where- needed).
> > Also since xen/paging.h included asm/paging.h and event.c only needs the
> > asm/paging.h part, I also changed xen/paging.h inclusion -> asm/paging.h
> > inclusion (include strictly -what's- needed).
> > But I can revert that if you want me to (not that important and these
> > little things are better done with automatic tools anyway), or should I
> > leave the change and update the commit message?
> Well, I'd leave the ultimate decision to the maintainers, but I'd say
> this doesn't belong in a patch dealing with formatting issues. I.e.
> I'm fine with the cleanup, but either under a different title (and
> with at least an abbreviated version of what you said above), or
> in a separate patch. Considering that you appear to do the same
> in later patches, perhaps consolidating all the #include adjustments
> into one patch would a good idea?
I tend to prefer organizing what's easy to understand/minor first (to 
let reviewers get rid of those in a flash), so I'd prefer to leave it in 
this patch with the other minor fixes and update the commit message instead.
But either way it's fine by me.

> > > > --- a/xen/include/xen/vm_event.h
> > > > +++ b/xen/include/xen/vm_event.h
> > > > @@ -85,7 +85,6 @@ void vm_event_monitor_guest_request(void);
> > > >    
> > > >    #endif /* __VM_EVENT_H__ */
> > > >    
> > > > -
> > > >    /*
> > > >     * Local variables:
> > > >     * mode: C
> > > Why don't you remove the other stray blank line at once?
> > What stray line? Shouldn't there be -one- blank line between the #endif
> > and the local vars block?
> > Looking @ other files that rule seems to hold...
> Oh, you're right. I thought I had frequently seen it the other way
> around (and it would seem more logical to me), but I must have been
> misremembering.
>
> Jan
Thanks,
Corneliu.

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