Hi Geert,
On 10/10/2023 17:52, Geert Uytterhoeven wrote:
> Hi Matt,
>
> On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@xxxxxxxxxx> wrote:
>> On 10/10/2023 00:56, Jakub Kicinski wrote:
>>> Add a section to netdev maintainer doc encouraging reviewers
>>> to chime in on the mailing list.
>>>
>>> The questions about "when is it okay to share feedback"
>>> keep coming up (most recently at netconf) and the answer
>>> is "pretty much always".
>>>
>>> Extend the section of 7.AdvancedTopics.rst which deals
>>> with reviews a little bit to add stuff we had been recommending
>>> locally.
>>
>> Good idea to encourage everybody to review, even the less experimented
>> ones. That might push me to send more reviews, even when I don't know
>> well the area that is being modified, thanks! :)
>>
>> (...)
>>
>>> diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
>>> index bf7cbfb4caa5..415749feed17 100644
>>> --- a/Documentation/process/7.AdvancedTopics.rst
>>> +++ b/Documentation/process/7.AdvancedTopics.rst
>>> @@ -146,6 +146,7 @@ pull. The git request-pull command can be helpful in this regard; it will
>>> format the request as other developers expect, and will also check to be
>>> sure that you have remembered to push those changes to the public server.
>>>
>>> +.. _development_advancedtopics_reviews:
>>>
>>> Reviewing patches
>>> -----------------
>>> @@ -167,6 +168,12 @@ comments as questions rather than criticisms. Asking "how does the lock
>>> get released in this path?" will always work better than stating "the
>>> locking here is wrong."
>>
>> The paragraph just above ("it is OK to question the code") is very nice!
>> When I'm cced on some patches modifying some code I'm not familiar with
>> and there are some parts that look "strange" to me, I sometimes feel
>> like I only have two possibilities: either I spend quite some time
>> understanding that part or I give up if I don't have such time. I often
>> feel like I cannot say "I don't know well this part, but this looks
>> strange to me: are you sure it is OK to do that in such conditions?",
>> especially when the audience is large and/or the author of the patch is
>> an experienced developer.
>
> Yes you can (even experienced developers can make mistakes ;-)!
Thank you for your reply!
> If it is not obvious that something is safe, it is better to point it
> out, so the submitter (or someone else) can give it a (second) thought.
> In case it is safe, and you didn't miss the ball completely, it probably
> warrants a comment in the code, or an improved patch description.
Indeed, good point!
It is good then to have that written in the doc -- I only discovered it
recently -- because, at least for me, it is easy to think that
experienced developers never make mistakes ( ;) ) and questioning their
code can only be done if we have double or triple checked that there is
likely an issue :)
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
