From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eu-smtp-delivery-143.mimecast.com (eu-smtp-delivery-143.mimecast.com [146.101.78.143]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0FC6F1A1E60 for ; Tue, 11 Oct 2016 05:29:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=rTSBhw69eSOqjzHOt8Xk8geC5u3u23FQfcT9w3Ul9Eg=; b=JVG3PPdavJAoy5tGGjuwEqMnjA89CxK0jO8msIcRfDHzOWh5KTQKOwwll3B9gXcVilnWRnX6fvr6XSDxoNJXZo28D7QuZdXXUw9wcWy+VsTrSwZBTftt5nWbEa6vTa42PVnjO4OUz6aFEzZlbkNzQ8KODG4f672llGU9WCqCE48= Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-ve1eur02lp0052.outbound.protection.outlook.com [213.199.154.52]) (Using TLS) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-31-vd4uHvcIO7iFMLSj3NVJOg-1; Tue, 11 Oct 2016 13:29:07 +0100 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com (10.169.247.16) by AM5PR0801MB1763.eurprd08.prod.outlook.com (10.169.247.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.649.16; Tue, 11 Oct 2016 12:29:06 +0000 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) by AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) with mapi id 15.01.0649.024; Tue, 11 Oct 2016 12:29:06 +0000 From: Evan Lloyd To: Leif Lindholm CC: "edk2-devel@ml01.01.org" , "ard.biesheuvel@linaro.org" , "ryan.harkin@linaro.org" Thread-Topic: [PATCH 3/3] ArmPlatformPkg: Remove UINTN cast when setting BaudRate. Thread-Index: AQHSIykqFiPwnANMQUOBBx5dsqFG56CjAh2ggAAclACAAA+3EA== Date: Tue, 11 Oct 2016 12:29:06 +0000 Message-ID: References: <20160921203315.11204-1-evan.lloyd@arm.com> <20160921203315.11204-4-evan.lloyd@arm.com> <20161010190438.GN3471@bivouac.eciton.net> <20161011112754.GR3471@bivouac.eciton.net> In-Reply-To: <20161011112754.GR3471@bivouac.eciton.net> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [217.140.96.140] x-ms-office365-filtering-correlation-id: 70da2241-5aff-4022-a607-08d3f1d231a3 x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1763; 7:WKgrurQnH1vGaRIYIXrTAY0XTCnyBqPNnX5FzmdY4m0wWbE+Cc3SUaGmD6n4cOFT1vzSzkaFw43GIkgYpCpRZU+vN76qe2ViqlYuepSHhAkA7HFiNlHUNeRE5iykJYliHXV1IUwb44A8FCUERoOt+CqlEBUT9RboZxqVTRWsGvyPEkwDhVOR6zT0IxFfzP/GmSFlL3hCjFQ4tSfsqKfL/sygxyB+RfrBIGWBet9AnmdGJoMFXXpQDYSmxIRUQR8nCa0tyMKctMBoqTjSBCJGFtJhwwrd0B/tNmz76dIxs2RkC0l+xE5raAeMd+4iA3Wom0jkvBDNPO1IMDkxPw2oIb3ZHdLO+BMct6lxHqLyYv8=; 20:yTF2onm1dmtzql3mGS82tXHQOOP5wfVvtAPTjY9ZeJu/xix278CGufabjA3HULQ0031DLAdLcDnTatczB9iqT4L3+fXdaRSIg23FWs2UCmOjRD+8mwb5k1ePJ6dNOR6fHI6qrALXiAKECqRa/tg2/ZimFG1Ulf/kWGHRgbtPoYA= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM5PR0801MB1763; x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040176)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6055026); SRVR:AM5PR0801MB1763; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1763; x-forefront-prvs: 00922518D8 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(24454002)(40434004)(13464003)(189002)(199003)(97736004)(3660700001)(66066001)(68736007)(5890100001)(189998001)(9686002)(2950100002)(3280700002)(19580395003)(19580405001)(6916009)(2900100001)(15975445007)(586003)(5660300001)(8936002)(102836003)(76576001)(92566002)(87936001)(7696004)(2906002)(3846002)(77096005)(6116002)(4326007)(110136003)(86362001)(105586002)(10400500002)(93886004)(122556002)(11100500001)(54356999)(8676002)(81156014)(106356001)(5002640100001)(74316002)(81166006)(305945005)(33656002)(50986999)(7736002)(76176999)(101416001)(106116001)(7846002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1763; H:AM5PR0801MB1762.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Oct 2016 12:29:06.5955 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1763 X-MC-Unique: vd4uHvcIO7iFMLSj3NVJOg-1 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 12:29:12 -0000 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Thanks, Leif. Some interesting points, and I agree and will strive to comply. I only point out that there MAY be a general pressure to not bother "tidyin= g" where trivia are observed. That is, of course, difficult to prove or quantify. Regards, Evan >-----Original Message----- >From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >Sent: 11 October 2016 12:28 >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 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 >> ... IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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.