From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 144D22274F3C4 for ; Thu, 12 Apr 2018 11:49:00 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D75A8406FA17; Thu, 12 Apr 2018 18:48:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-142.rdu2.redhat.com [10.10.120.142]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3D7B8D7DF8; Thu, 12 Apr 2018 18:48:59 +0000 (UTC) To: Leif Lindholm Cc: "edk2-devel@lists.01.org" References: <20180412005540.26651-1-lersek@redhat.com> <20180412172318.jtugn7p3w2qgr24y@bivouac.eciton.net> <20180412181359.rvt2s4fhyuxmj465@bivouac.eciton.net> From: Laszlo Ersek Message-ID: <39a257c1-a336-45f6-d3ec-cd295cb812de@redhat.com> Date: Thu, 12 Apr 2018 20:48:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180412181359.rvt2s4fhyuxmj465@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 12 Apr 2018 18:48:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 12 Apr 2018 18:48:59 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: derailing into patch style discussion X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Apr 2018 18:49:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/12/18 20:13, Leif Lindholm wrote: > Since you already have my r-b on the set, I'll pick up the style > topic, partly because I'm not sure if I've ever explained my > thinking publicly in words that anyone other than Ard understands. > > On Thu, Apr 12, 2018 at 07:45:19PM +0200, Laszlo Ersek wrote: >>> Well, there are a couple of places where I could nitpick on >>> alphabetical sorting of includes, >> >> And, believe me, you would have my total agreement :), but in such >> cases, there's always a fork in the road: (a) add a separate patch that >> first sorts the includes and [LibraryClasses], without functional >> changes, or (b) just stick with the existing disorder, and get in and >> out as surgically as possible. My personal preference is (a), but it has >> drawn disagreement -- even accusations of pedantry :) :) --, and/or >> suggestions to squash such patches with functional changes, in the past, >> so I trod more lightly now. Rest assured, I didn't *miss* those places, >> I just elected to close my eyes! ;) > > Right :) > > So, yes - I do strongly agree with the idea of keeping functionality > and style changes separate (ask Evan), so that wasn't exactly what I > was referring to. > > In general my internal "optimal" situation is one where the purely > functional diff leaves the code in a (quite subjectively, since it's > still not conformant) better state on average than it was. > > My subjective mark of optimal is that which would minimise any > subsequent patch that was _only_ a style cleanup. > > To pick an example from this set (1/10): > diff --git a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf > b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf > index 6deb8c3f675c..61ad89af2758 100644 > --- a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf > +++ b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf > @@ -41,14 +41,14 @@ [LibraryClasses] > PrintLib > UefiDriverEntryPoint > IoLib > ArmLib > > [Protocols] > - gHardwareInterruptProtocolGuid > - gEfiCpuArchProtocolGuid > + gHardwareInterruptProtocolGuid ## PRODUCES > + gEfiCpuArchProtocolGuid ## CONSUMES ## NOTIFY > > --- > This one is pretty straightforward - without touching any non-modified > lines, these _could_ be reordered alphabetically. > > (Yes, that change may have functional side-effects, but only on > undefined behaviour, so no different from rebasing to a version with > newer BaseTools can give you.) > > Where the minimum diff logic comes in is in situations like (6/10): > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > index 812dafd065b2..c40ac27a6599 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > @@ -28,12 +28,13 @@ [Sources.common] > NorFlashBlockIoDxe.c > > [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > ArmPlatformPkg/ArmPlatformPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > > --- > Applying my "optimal" rule here would have meant inserting the new > line before MdePkg/MdeModulePkg .decs instead. That way a cleanup > patch would end up doing > > [Packages] > + ArmPlatformPkg/ArmPlatformPkg.dec > EmbeddedPkg/EmbeddedPkg.dec > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > - ArmPlatformPkg/ArmPlatformPkg.dec > > instead of > > [Packages] > - MdePkg/MdePkg.dec > - MdeModulePkg/MdeModulePkg.dec > ArmPlatformPkg/ArmPlatformPkg.dec > EmbeddedPkg/EmbeddedPkg.dec > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > > > Anyway, this is all an aside - I just thought I would give you an > insight into the mind of Leif. I do see the point -- basically, even if we don't imlement separate cleanups, and just add the functional changes on top of whatever we already have, if there are multiple ways to keep the functional changes purely functional and focused, we had better select the one that at least doesn't make the style worse. Using your [Packages] example, it's possible *not* to increase the disorder, without actually sorting those lines. I fully admit this eluded me -- I considered "messy" all-or-nothing. I'll try to remember this next time. (It's honestly a bit difficult for me, because if I take the time & effort to be considerate like this, I'd mostly (though perhaps not always) just write the patch that cleans up first.) Thanks! Laszlo