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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

> 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!