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

[Leif Lindholm <leif.lindholm@linaro.org>]

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
> 
> >-----Original Message-----
> >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >Sent: 10 October 2016 20:05
> >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 Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.lloyd@arm.com wrote:
> >> From: Alexei <Alexei.Fedorov@arm.com>
> >>
> >> SerialPortInitialize() set the BaudRate variable (type UINT64) as:
> >> BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>
> >> This commit fixes a potential problem on ARM 32-bit builds, where the
> >> UINTN type is defined as UINT32, by removing the cast:
> >>
> >> BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>
> >> Note - a minor whitespace correction is rolled into this commit.
> >
> >I can unroll it for you before committing, but I'm not going to leave
> >the history in a state where it looks like a FixedPcdGet8 was modified
> >by a commit with the title "Remove UINTN cast when setting BaudRate.".
> >
> >For the fix:
> >Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> >Let me know how you want to deal with the whitespace change.
> >
> >/
> >    Leif
> >
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
> >> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> >> ---
> >>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git
> >a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> index
> >5dce852d90f9cafb828d81dae39d03451ea608e2..4a24eded0e7d0f91270bf778
> >cf1d89b6c809d0b2 100644
> >> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> >> @@ -41,11 +41,11 @@ SerialPortInitialize (
> >>    UINT8               DataBits;
> >>    EFI_STOP_BITS_TYPE  StopBits;
> >>
> >> -  BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
> >> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> >>    ReceiveFifoDepth = 0;         // Use default FIFO depth
> >>    Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
> >>    DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
> >> -  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8
> >(PcdUartDefaultStopBits);
> >> +  StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8
> >(PcdUartDefaultStopBits);
> >>
> >>    return PL011UartInitializePort (
> >>             (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
> >> --
> >> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >>
> 
> 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.
>