Re: [XEN PATCH v2] x86: make function declarations consistent with definitions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Jul 2023, Jan Beulich wrote:
> On 07.07.2023 00:29, Stefano Stabellini wrote:
> > On Thu, 6 Jul 2023, Jan Beulich wrote:
> >> On 06.07.2023 01:22, Stefano Stabellini wrote:
> >>> On Tue, 4 Jul 2023, Jan Beulich wrote:
> >>>> On 04.07.2023 12:23, Federico Serafini wrote:
> >>>>> Change mechanically the parameter names and types of function
> >>>>> declarations to be consistent with the ones used in the corresponding
> >>>>> definitions so as to fix violations of MISRA C:2012 Rule 8.3 ("All
> >>>>> declarations of an object or function shall use the same names and type
> >>>>> qualifiers") and MISRA C:2012 Rule 8.2 ("Function types shall be in
> >>>>> prototype form with named parameters").
> >>>>>
> >>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> >>>>
> >>>> On top of my earlier remark (when this was part of a series):
> >>>
> >>> I am not addressing specifically this comment. I am trying to build a
> >>> common understanding on how to do things so that we can go faster in the
> >>> future.
> >>>
> >>> In general, as discussed at Xen Summit, in order to successfully merge
> >>> large numbers of changes in the coming weeks we should try to keep
> >>> mechanical changes mechanical. Separate non-mechanical changes into
> >>> different patches.
> >>>
> >>> This patch is large but mechanical. If I understand you correctly, you
> >>> are asking:
> >>> 1) to split the patch into smaller patches
> >>> 2) make a couple of non-mechanical changes described below
> >>>
> >>>
> >>> For 1), in my opinion it is not necessary as long as all changes remain
> >>> mechanical. If some changes are not mechanical they should be split out.
> >>> So if you are asking non-mechanical changes in 2), then 2) should be
> >>> split out but everything else could stay in the same patch.
> >>>
> >>> If you'd still like the patch to be split, OK but then you might want to
> >>> suggest exactly how it should be split because it is not obvious: all
> >>> changes are similar, local, and mechanical. I for one wouldn't know how
> >>> you would like this patch to be split.
> >>
> >> So I gave a clear reason and guideline how to split: To reduce the Cc
> >> list of (because of requiring fewer acks for) individual patches, and
> >> to separate (possibly) controversial from non-controversial changes.
> >> This then allows "easy" changes to go in quickly.
> >>
> >> I realize that what may be controversial may not always be obvious,
> >> but if in doubt this can be addressed in a v2 by simply omitting such
> >> changes after a respective comment was given (see also below).
> > 
> > So the guideline is to separate by maintainership, e.g.
> > x86/arm/common/vpci
> > 
> > Also separate out anything controversial and/or that receives feedback
> > so it is not mechanical/straightforward anymore.
> > 
> > 
> >>> For 2), I would encourage you to consider the advantage of keeping the
> >>> changes as-is in this patch, then send out a patch on top the way you
> >>> prefer. That is because it costs you more time to describe how you
> >>> would like these lines to be changed in English and review the full
> >>> patch a second time, than change them yourself and anyone could ack them
> >>> (feel free to CC me).
> >>>
> >>> For clarity: I think it is totally fine that you have better suggestions
> >>> on parameter names. I am only pointing out that providing those
> >>> suggestions as feedback in an email reply is not a very efficient way to
> >>> get it done.
> >>
> >> What you suggest results in the same code being touched twice to
> >> achieve the overall goal (satisfy Misra while at the same time not
> >> making the code any worse than it already is). I'd like to avoid this
> >> whenever possible, so my preference would be that if the English
> >> description isn't clear, then the respective change would best be
> >> omitted (and left to be addressed separately).
> > 
> > Yes, I think that would work. Basically the process could look like
> > this:
> > 
> > - contributor sends out a patch with a number of mechanical changes
> > - reviewer spots a couple of things better done differently
> > - reviewer replies with "drop this change, I'll do it" no further
> >   explanation required
> > - in parallel: contributor sends out v2 without those changes for the
> >   reviewer to ack
> > - in parallel: reviewer sends out his favorite version of the changes
> >   for anyone to ack (assuming he is the maintainer)
> 
> For this last point, I don't see it needing to happen in parallel.
> Reviewers may be busy with other things, and making less mechanical
> changes can easily be done a little later. The overall count of
> violations is still going to decrease.

OK. Another suggestion along these lines is that if a revision of a
patch is OK except for 2 changes, those 2 changes could be removed on
commit to avoid another re-submit and re-review.

E.g. a patch has 50 fixes. 2 of these fixes are wrong, the rest are OK.
The maintainer/committer commits the patch with 48 fixes, removing the 2
unwanted fixes.

Keep in mind that resubmissions of these MISRA C patches also cause more
work for the reviewers/maintainers. I think we should try to find ways
to decrease the overall workload of everyone involved.