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
One could rephrase that as "does a tidy git log outweigh good code quality?"
>From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>Sent: 10 October 2016 20:05
>To: Evan Lloyd
>Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org;
>Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when
>On Wed, Sep 21, 2016 at 09:33:15PM +0100, evan.ll...@arm.com wrote:
>> From: Alexei <alexei.fedo...@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.lindh...@linaro.org>
>Let me know how you want to deal with the whitespace change.
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Alexei Fedorov <alexei.fedo...@arm.com>
>> Signed-off-by: Evan Lloyd <evan.ll...@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
>> @@ -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
>> + StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8
>> return PL011UartInitializePort (
>> (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
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.
edk2-devel mailing list