Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate.

[Evan Lloyd <Evan.Lloyd@arm.com>]

Thanks, Leif.
Some interesting points, and I agree and will strive to comply.
I only point out that there MAY be a general pressure to not bother "tidying" where trivia are observed.
That is, of course, difficult to prove or quantify.

Regards,
Evan

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: 11 October 2016 12:28
>To: Evan Lloyd
>Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
>ryan.harkin@linaro.org
>Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
>setting BaudRate.
>
>On Tue, Oct 11, 2016 at 10:23:12AM +0000, Evan Lloyd wrote:
>> Please feel free to fix the space change as you see fit and proper,
>> as it was just incidental tidying up.
>
>Thanks.
>
>> It would be good to have a discussion about the general position,
>> though.
>> There are, I am sure, sound reasons for not rolling these things
>> together (and I knew that, and shouldn't have, but...).
>> I understand some of those reasons, but I see some unfortunate
>> consequences too, so I'd like to play devil's advocate here.
>> One unfortunate effect is to discourage the submission of trivial
>> changes that would not in themselves justify the rigmarole of a
>> patch.
>
>In summary: the mixing of functional and non-functional modifications
>reduces the effectiveness of code review and increases maintainer
>overhead.
>
>> I know we would hope to do it all properly, but I don't think that
>> is how human nature works.  The cost/benefit comparison of adding or
>> removing a space (or other cosmetic change) as a separate patch is
>> not really worthwhile, so it is much easier to not "see" the
>> improvement.  Please note: I am not thinking solely of myself here -
>> I know others find the same thing.
>> Of course, if the intent is " to discourage the submission of
>> trivial changes" that would make a sort of sense from the
>> maintainer's perspective.
>
>Certainly not. But maintainers are not mind readers. Sure, if it's a
>trivial fix it won't take _that_ much longer to review the patch. But
>where's the limit on that? How likely am I to miss a code error (or a
>possible improvement) because I'm busy trying to find which bits are
>functional vs. non-functional changes?
>
>This is also why we sometimes ask for large functional patches to be
>subdivided.
>See also https://alexgaynor.net/2015/dec/29/shrinking-code-review/
>
>On the flip-side, I am very much happy to take non-functional-only
>patches to large swathes of files. So if you find that less tedious,
>you're welcome to collect up a bunch of these, squash them together
>into a single commit and submit every now and then.
>(Although backpedalling again, semantic changes to comments are best
>left separate.)
>
>> My position is that by making minor incidental improvement
>> relatively expensive the actual effect is to discourage it.
>> Does the benefit derived from discrete patches really override this
>> disadvantage?
>
>Yes.
>
>> One could rephrase that as "does a tidy git log outweigh good code
>> quality?"
>
>I would prefer to put it as "a tidy log and more easily reviewable
>patches are required for good code quality".
>
>Regards,
>
>Leif
>
>> Regards,
>> Evan
>>
...

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.