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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2023 23:28, Stefano Stabellini wrote:
> 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.

Sure, no concern in this regard from my side.

Jan

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