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.lindh...@linaro.org] > >Sent: 10 October 2016 20:05 > >To: Evan Lloyd > >Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org; > >ryan.har...@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.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. > > > >/ > > Leif > > > >> 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 > >> 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
