From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (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 8084F21A0913E for ; Mon, 22 May 2017 10:50:25 -0700 (PDT) Received: by mail-wm0-x22b.google.com with SMTP id 7so2555566wmo.1 for ; Mon, 22 May 2017 10:50:25 -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:content-transfer-encoding:in-reply-to :user-agent; bh=B7O5ybgLXJvpQBjL1lq8nxDAsjbJ/3OxCikQCQnnT08=; b=YjyghlC6l+9zBt+RlXMTZT1S5ZaXBdmJ1vLDRVJOSuxjyL9z+ZmGGawOsgAdLnhyNj //5qGEtIO12P3V/7rcH5hxl2QqLWmVJFXy+HBD58P4bSkcHHcG5F/mvV0qtN/r4zE/+k 1dMFah+Y92b5mqXt5ZdGbchkZ0m933A2HDMPY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=B7O5ybgLXJvpQBjL1lq8nxDAsjbJ/3OxCikQCQnnT08=; b=qVPkRZhPb9UBCMYOeUHgEoe7HLiSKNKq40GOGmjpPNYFEplaiIkrKM6tpqiBvY5wyn 2+MvWjnDcwwuccXJFHQbQAZf1fTtgSgF/fiyw9Ri2QZues2hLPsfk/PSmSuWNiNmS2eS Dmp3rQbpUvt7J88gOw9jb42Z3oB8krTlnlAPdhb42rFtQ4rgiqzgNo32aTuGCVdq69xs jUOdwCjjHYlZP4G8lloWThHK3x0dU9Si4sXI+XUNlmtxPcSzp62ICCo4Kd8QaOOFwLCU 3CEE02fTA/LdoThJGtXWwCdG3WI+my7698u1bu/wjJI5h0fv5dulEKuSXL5mZGkNbLAQ 4l3w== X-Gm-Message-State: AODbwcBHdkiSW+zll4Mv0z6hdF2par6s/hSRkU4QBJjz9gbKZIoQ2623 UvcRpC6onoZqDp2I X-Received: by 10.28.41.65 with SMTP id p62mr14532474wmp.32.1495475423991; Mon, 22 May 2017 10:50:23 -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 q16sm22730301wmg.2.2017.05.22.10.50.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 22 May 2017 10:50:23 -0700 (PDT) Date: Mon, 22 May 2017 18:50:21 +0100 From: Leif Lindholm To: Jordan Justen Cc: Laszlo Ersek , edk2-devel-01 , Ard Biesheuvel Message-ID: <20170522175021.GL1657@bivouac.eciton.net> 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> <149521314566.21878.13927782008260700274@jljusten-skl> MIME-Version: 1.0 In-Reply-To: <149521314566.21878.13927782008260700274@jljusten-skl> User-Agent: Mutt/1.5.23 (2014-03-12) 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: Mon, 22 May 2017 17:50:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Fri, May 19, 2017 at 09:59:06AM -0700, Jordan Justen wrote: > 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=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock)); > > > > >> + DEBUG (( > > > > >> + DEBUG_BLKIO, > > > > >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n", > > > > > > > > > > Notably this is still > 80 columns. Maybe? > > > > > > > > > > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) " > > > > > "> LastBlock=%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 per > > > > 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. Yes - had that string appeared as a new entity (instead of a modification of existing code, with Laszlo being the good citizen to make sure the modified lines complied as well as possible), that would most likely have been my feedback. > > > 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’t 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. Most definitely. / Leif