From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-x232.google.com (mail-qt0-x232.google.com [IPv6:2607:f8b0:400d:c0d::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B161E1A1E82 for ; Tue, 11 Oct 2016 04:27:57 -0700 (PDT) Received: by mail-qt0-x232.google.com with SMTP id q7so10175211qtq.1 for ; Tue, 11 Oct 2016 04:27:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=wzWSdWqf5IBnx9zryxkNwxjQGIGgZbWrEyMzOg8tvSI=; b=X2eVXhP53NTem7b8QxC0kc0NJtZn+6Zpys70l+RViJbMymffFD+Rl6kPS5m2EyyPHH 8rk9KP/j4lkebSYsz+Bqow7NUnxoKg268FIa6i86sf8KtyMOxL625LCbHs/31fK6d/U6 sVzRuso4xSudCCEtrtJQF39BIZwJFDA+olVCw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=wzWSdWqf5IBnx9zryxkNwxjQGIGgZbWrEyMzOg8tvSI=; b=hfBd/PaGZZ7j0xQHNOYwPKDc0jTKQpm7p3dWncFWtAShF+18feSBCivSyrFtFKFkuT cgPz+zqF6NDKd2bKV0LtMEd0uXcgmwgom/cC+25v6LR1UmhRYNuefWgnHPeRqACNu4c/ 1ggo+atySwkJf+J0RaB/s+CbOYJq/Mh9mr+mIbT+gjxexMduTAQ2BNR+s582AbujrYRP if+6dW9H3yR2AVY66Z7Nq2S3BwWrWc617fCE3v6mUXjmaVo5Vb73+cWl0OBthuC5eSRl k1dR6S6R8/ifSE3T963n1VOMKARvlkdiR+YLyE4R6MnAECBDZvTsplIMgCLww9d2vKjZ OVwA== X-Gm-Message-State: AA6/9Rkdyik5oCdMroQJcrFLdrw1H9I1TlIdgp83S8Iq1Nu8kj3/NyH7h22BFle7ZtdXRGJY X-Received: by 10.194.24.199 with SMTP id w7mr4427073wjf.197.1476185276522; Tue, 11 Oct 2016 04:27:56 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id va3sm5672883wjb.18.2016.10.11.04.27.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Oct 2016 04:27:55 -0700 (PDT) Date: Tue, 11 Oct 2016 12:27:54 +0100 From: Leif Lindholm To: Evan Lloyd Cc: "edk2-devel@ml01.01.org" , "ard.biesheuvel@linaro.org" , "ryan.harkin@linaro.org" Message-ID: <20161011112754.GR3471@bivouac.eciton.net> References: <20160921203315.11204-1-evan.lloyd@arm.com> <20160921203315.11204-4-evan.lloyd@arm.com> <20161010190438.GN3471@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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:27:58 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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.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. >