From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 A922421A16EFC for ; Fri, 19 May 2017 09:59:07 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP; 19 May 2017 09:59:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,365,1491289200"; d="scan'208";a="1171763371" Received: from rfink-mobl.amr.corp.intel.com (HELO localhost) ([10.255.77.177]) by fmsmga002.fm.intel.com with ESMTP; 19 May 2017 09:59:06 -0700 MIME-Version: 1.0 To: Leif Lindholm Message-ID: <149521314566.21878.13927782008260700274@jljusten-skl> From: Jordan Justen In-Reply-To: <20170519100756.GB1657@bivouac.eciton.net> Cc: Laszlo Ersek , edk2-devel-01 , Ard Biesheuvel References: <20170518150427.16435-1-lersek@redhat.com> <20170518150427.16435-2-lersek@redhat.com> <149512810792.14216.11044391728020039744@jljusten-skl> <149514029896.16116.5092031537929111173@jljusten-skl.jf.intel.com> <20170519100756.GB1657@bivouac.eciton.net> User-Agent: alot/0.5.1 Date: Fri, 19 May 2017 09:59:06 -0700 Subject: Re: [PATCH 1/8] ArmPlatformPkg/NorFlashDxe: correct NumOfLba vararg type in EraseBlocks() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 May 2017 16:59:07 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-05-19 03:07:56, Leif Lindholm wrote: > On Thu, May 18, 2017 at 01:44:58PM -0700, Jordan Justen wrote: > > On 2017-05-18 12:29:09, Laszlo Ersek wrote: > > > On 05/18/17 19:21, Jordan Justen wrote: > > > > On 2017-05-18 08:04:20, Laszlo Ersek wrote: > > > >> // All blocks must be within range > > > >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba= =3D%ld + NumOfLba=3D%d - 1 ) > LastBlock=3D%ld.\n", Instance->StartLba + St= artingLba, NumOfLba, Instance->Media.LastBlock)); > > > >> + DEBUG (( > > > >> + DEBUG_BLKIO, > > > >> + "FvbEraseBlocks: Check if: ( StartingLba=3D%ld + NumOfLba= =3D%Lu - 1 ) > LastBlock=3D%ld.\n", > > > > = > > > > Notably this is still > 80 columns. Maybe? > > > > = > > > > "FvbEraseBlocks: Check if: ( StartingLba=3D%ld + NumOfLba=3D%= Lu - 1 ) " > > > > "> LastBlock=3D%ld.\n", > > > = > > > This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has > > > extremely long lines, the longest one (line 774) has 172 columns. I > > > broke up the above DEBUG so that it would at least fit in 120 chars p= er > > > line (which is the "second level" recommendation in the coding spec). > > = > > Personally, I don't agree with that secondary 120 char rule. If we > > ever get the style guide into an 'open source' process, I'd like to > > suggest removing it. (But, it'll probably get shot down. :\ ) > = > Oh, I'm all for that one. And the style guide is definitely in need of > a shake-up. > = > But I consider violating line length restrictions less bad than making > user-(or in this case developer)-visible strings harder to search for. While of less concern, I would say the style guide should recommend that logged debug messages should attempt to be less than 80 chars. That could help here. > > Ah. I guess it is fine for a package maintainer to occasionally decide > > to bend the rules for their package. > = > For this to be bending, it would require the 120-character rule to not > exist. Yeah. It is very silly to say, we prefer 80 chars, but if it's too hard then 120 can be used. :) The latest coding standard says: "Preferably, limit line lengths to 80 columns or less. When this doesn=E2=80=99t leave sufficient space for a good postfix style comment, extend the line to a total of 120 columns." This seems to imply that the extension beyond 80 chars is reserved for postfix comments. This seems like a poor reason to push past 80. It would be better to comment above the line instead. -Jordan