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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> 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). (I hope it is obvious
enough why [largely needlessly] touching the same code twice isn't
nice.)

Jan