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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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)

This should work well with MISRA C because they are a large number of
changes but each of them very simple, so I really believe it will take
less time for the maintainer to write a patch than try to explain in
English and more back and forth.

I think this is less work for anyone involved. Let's give it a try!