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

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

Hi Leif.
Please feel free to fix the space change as you see fit and proper, as it was just incidental tidying up.

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.
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.
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?
One could rephrase that as "does a tidy git log outweigh good code quality?"

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.