From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C41BC1A1DE9 for ; Tue, 11 Oct 2016 04:34:11 -0700 (PDT) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5514EC04B30A; Tue, 11 Oct 2016 11:34:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-32.phx2.redhat.com [10.3.116.32]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9BBY8Ou016135; Tue, 11 Oct 2016 07:34:08 -0400 To: Evan Lloyd , Leif Lindholm References: <20160921203315.11204-1-evan.lloyd@arm.com> <20160921203315.11204-4-evan.lloyd@arm.com> <20161010190438.GN3471@bivouac.eciton.net> Cc: "edk2-devel@ml01.01.org" , "ryan.harkin@linaro.org" , "ard.biesheuvel@linaro.org" From: Laszlo Ersek Message-ID: <3cf94dc0-c70c-f7f7-bf6f-51f8a249227a@redhat.com> Date: Tue, 11 Oct 2016 13:34:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 11 Oct 2016 11:34:11 +0000 (UTC) Subject: Re: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2016 11:34:12 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 >>> >>> 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 >> >> 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 >>> Signed-off-by: Evan Lloyd >>> --- >>> 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 >