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

[Laszlo Ersek <lersek@redhat.com>]

On 10/11/16 12:23, Evan Lloyd wrote:
> Hi Leif.
> Please feel free to fix the space change as you see fit and proper,
> as it was just incidental tidying up.

I would simply drop that hunk for now. While I personally prefer the
no-space form, and stick with it consistently in all code I write, other
edk2 developers are consistent in their opposite use of the space.

The edk2 coding standards doc I'm looking at now (Version 2.1, 30
October 2015 -- it could be old) does not regulate casts in this aspect.
Looking for examples rather than for explicit rules, we find both flavors:

* In 5.7.2.3 "Comparison of unsigned integer types to be >=0 is
permitted", we have

  if ((INTN)foo >= 0) { ...

that is, no space.

* In 5.7.2.4 "The ordering of terms in predicate expressions may impact
performance significantly", there's

  if (( LogEntryArray[Index].Handle == (EFI_PHYSICAL_ADDRESS) (UINTN)
Handle)

with one space afte (EFI_PHYSICAL_ADDRESS) and (UINTN) each.

Again, I strongly prefer the no-space form -- the cast operator is one
of the most strongly binding operators in C, and the space, common to
edk2, has actually caused developers to mis-read code and add bugs --,
but in this specific case, removing just this one space looks arbitrary
to me.

> 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.

In this case, I agree with both of you -- squashing the space change is
not nice, and writing a one-liner patch for the space change is also
overkill. For that reason, I suggest to simply drop the change. The
current spacing -- while inferior, in my persional opinion -- is not a
bug, and it matches existent edk2 practice. I agree the spacing does not
match the direct neighborhood -- if you feel strongly about that, it
might justify a separate patch.

> 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.

"Trivial" improvements are definitely welcome (as far as I'm concerned,
but I'm quite sure Leif agrees :)), as long as they are well-focused.

> My position is that by making minor incidental improvement relatively
> expensive the actual effect is to discourage it.

I agree with your assessment.

> Does the benefit derived from discrete patches really override this
> disadvantage?

In my opinion: yes, it does.

> One could rephrase that as "does a tidy git log outweigh good code quality?"

In my opinion: yes, it does.

I think you may have considered git-log only. Please consider git-blame
and git-bisect too:

- With git-blame, you go from source code to commit message (and other
source code) -- that is, you pick a line of code, and would like to see
the explanation for it (commit message) and any logically pertaining
changes (other hunks in the same patch). Unrelated code hunks disturb
this process.

- With git-bisect, you have a regression (or, in a reverse bisectection,
an unexpected / unidentified fix) that you'd like to pinpoint. Keyword
"pin". The larger the patch "git-bisect" identifies, the less useful
"git-bisect" is to you -- after the bisection completes, one still has
to figure out why the identified patch causes the regression (or the
unexpected fix). Unrelated hunks are distracting in this process.

So, generally speaking:
- if "good code quality" is attainable by an actual bugfix, then that
certainly deserves a separate patch,
- if we downgrade "good code quality" to "tidy-looking code", then a
tidy git log certainly outweighs that (IMO).

In this particular case, I'd suggest to simply drop the space change, or
if we feel strongly about it, to include it in a separate patch. The
latter might feel awkward now, but it'll feel better in future blamings
and bisections.

(Sorry for the unsolicited opinion...)

Thanks
Laszlo

>> -----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.
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>