Re: [PATCH 1/8] ArmPlatformPkg/NorFlashDxe: correct NumOfLba vararg type in EraseBlocks()

[Leif Lindholm <leif.lindholm@linaro.org>]

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