Re: [PATCH V2 01/17] MdePkg/DebugLib.h: Add a new api DebugVPrint for DebugLib

[Jordan Justen <jordan.l.justen@intel.com>]

On 2019-03-18 08:09:36, Gao, Liming wrote:
> Jordan:
>   Coding style document CCS_2_1_Draft.pdf 5.1 General Rules, 5.1.1
>   Lines shall be 120 columns, or less. Preferably, limit line
>   lengths to 80 columns or less. So, I don't think the line length
>   bigger than 80 and less than 120 violates coding style documents.

I think this is clarified as an "exception" base: "When this doesn’t
leave sufficient space for a good postfix style comment, extend the
line to a total of 120 columns."

That doesn't apply to this case.

I have never seen a case where I thought this exception was useful in
EDK II. I think the exception should be removed, because if a true
exceptional case ever did arise, it would be so obvious, no one would
need to question it.

The only case I've seen (not in EDK II) where going beyond 80 columns
seemed to make some sense was a table processed by a build tool. In
that case the table has so many columns that line went beyond 80
columns.

There a good code-readability reasons for this limit. For one, we can
be almost certain that everyone will have 80 columns visible in their
editor. But, just as importantly, as the line gets too long, it
becomes more challenging to track back to the start of the next line.
(Think of newspaper columns, and imagine if they ran all the way
across the page.) I'm not sure 80 columns is the "perfect" number, but
it does seem that most agree it is probably close.

>   We should update wiki
>   https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C
>   to match the documentation. Limit line length to 120 characters
>   instead of 80 characters.

Since this is an "exception", and not normally helpful, I don't think
we should add it do the wiki. It will only add confusion and lead to
more code that doesn't follow the preferred limit.

-Jordan

> 
> > -----Original Message-----
> > From: Justen, Jordan L
> > Sent: Monday, March 18, 2019 2:21 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Michael Turner
> > <Michael.Turner@microsoft.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [edk2] [PATCH V2 01/17] MdePkg/DebugLib.h: Add a new api DebugVPrint for DebugLib
> > 
> > On 2019-03-17 17:14:40, Gao, Zhichao wrote:
> > > > -----Original Message-----
> > > > From: Justen, Jordan L
> > > > Sent: Friday, March 15, 2019 2:16 PM
> > > >
> > > > On 2019-03-14 22:17:33, Zhichao Gao wrote:
> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
> > > > >
> > > > > Add a new api DebugVPrint prototype definition in the
> > > > > DebugLib header file. This api would expose a print
> > > > > routine with VaList parameter.
> > > >
> > > > These lines seem to be fairly short, with the longest be 54 chars. I guess not a
> > > > problem, but by the recommendation says they could be up to 75 in length.
> > > >
> > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> > > > Format
> > > >
> > >
> > > It is hard for someone to control the characters in a line.
> > 
> > I guess that depends on your editor. :)
> > 
> > > Making the text massage readable base on the rules make more sense.
> > > It is easier to control the chars number within 75. Line length less
> > > than 76 characters is legal.
> > 
> > Yes, I mentioned it is legal, but it is noticably short. I don't think
> > it is so short that it is really a problem, so feel free to leave it
> > if you prefer.
> > 
> > > >
> > > > According to the style guide:
> > > >
> > > >   "Preferably, limit line lengths to 80 columns or less."
> > > >
> > > > https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C
> > > >
> > > > https://github.com/tianocore-
> > > > docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf
> > > >
> > > > But, this line uses 92 columns.
> > > >
> > > > I think there are similar cases in other patches.
> > >
> > > Right. This sentence I copied from the comment of DebugPrint
> > > function which might be designed for a long time. The CCS might not
> > > be designed as such style at that time. For this, it's better to
> > > change both of the two sentences.
> > > By the way, the whole edk2 repo may contain a lot of lines violated
> > > this rule. It takes effort to fix them all.
> > 
> > I agree there are many violations of this in edk2.
> > 
> > Usually, it's not too important to go around fixing the issues that
> > already happened, but if you are changing or adding some code, then
> > you can try to make sure the new lines you add are good.
> > 
> > -Jordan