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

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Justen, Jordan L
> Sent: Friday, March 15, 2019 2:16 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-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. 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.

> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h
> > b/MdePkg/Include/Library/DebugLib.h
> > index e6a7a357b2..51d89bbd52 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -8,7 +8,7 @@
> >    of size reduction when compiler optimization is disabled. If
> MDEPKG_NDEBUG is
> >    defined, then debug and assert related macros wrapped by it are the
> NULL implementations.
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  This program and the accompanying materials are licensed and made
> > available under  the terms and conditions of the BSD License that
> accompanies this distribution.
> >  The full text of the license may be found at @@ -101,6 +101,29 @@
> > DebugPrint (
> >    );
> >
> > +/**
> > +  Prints a debug message to the debug output device if the specified error
> level is enabled.
> 
> 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.

> 
> > +
> > +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib
> > + function  GetDebugPrintErrorLevel (), then print the message
> > + specified by Format and the  associated variable argument list to the
> debug output device.
> > +
> > +  If Format is NULL, then ASSERT().
> > +
> > +  @param  ErrorLevel    The error level of the debug message.
> > +  @param  Format        Format string for the debug message to print.
> > +  @param  VaListMarker  VA_LIST marker for the variable argument list.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +DebugVPrint (
> > +  IN  UINTN        ErrorLevel,
> > +  IN  CONST CHAR8  *Format,
> > +  IN  VA_LIST       VaListMarker
> > +  );
> > +
> > +
> >  /**
> >    Prints an assert message containing a filename, line number, and
> description.
> >    This may be followed by a breakpoint or a dead loop.
> > @@ -221,6 +244,7 @@ DebugClearMemoryEnabled (
> >    VOID
> >    );
> >
> > +
> 
> What's going on here? It seems like maybe extra lines are added that have
> nothing to do with this patch.
> 
> -Jordan
> 

For this library DebugLib, seems it prefers to put two blank lines between functions. I just want to make it tidy. Maybe it is an inappropriate behavior. But there are no rules for how much blank lines should be putted between functions. For my own, one single blank line is enough. Both adding blank lines and deleting blank lines seem inappropriate. Or just stay the original style?

Thanks,
Zhichao

> >  /**
> >    Returns TRUE if any one of the bit is set both in ErrorLevel and
> PcdFixedDebugPrintErrorLevel.
> >
> > @@ -236,6 +260,7 @@ DebugPrintLevelEnabled (
> >    IN  CONST UINTN        ErrorLevel
> >    );
> >
> > +
> >  /**
> >    Internal worker macro that calls DebugAssert().
> >
> > @@ -273,6 +298,7 @@ DebugPrintLevelEnabled (
> >  #define _DEBUG(Expression)   DebugPrint Expression
> >  #endif
> >
> > +
> >  /**
> >    Macro that calls DebugAssert() if an expression evaluates to FALSE.
> >
> > @@ -299,6 +325,7 @@ DebugPrintLevelEnabled (
> >    #define ASSERT(Expression)
> >  #endif
> >
> > +
> >  /**
> >    Macro that calls DebugPrint().
> >
> > @@ -322,6 +349,7 @@ DebugPrintLevelEnabled (
> >    #define DEBUG(Expression)
> >  #endif
> >
> > +
> >  /**
> >    Macro that calls DebugAssert() if an EFI_STATUS evaluates to an error
> code.
> >
> > @@ -348,6 +376,7 @@ DebugPrintLevelEnabled (
> >    #define ASSERT_EFI_ERROR(StatusParameter)  #endif
> >
> > +
> >  /**
> >    Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error
> code.
> >
> > @@ -375,6 +404,7 @@ DebugPrintLevelEnabled (
> >    #define ASSERT_RETURN_ERROR(StatusParameter)
> >  #endif
> >
> > +
> >  /**
> >    Macro that calls DebugAssert() if a protocol is already installed in the
> >    handle database.
> > @@ -418,6 +448,7 @@ DebugPrintLevelEnabled (
> >    #define ASSERT_PROTOCOL_ALREADY_INSTALLED(Handle, Guid)  #endif
> >
> > +
> >  /**
> >    Macro that marks the beginning of debug source code.
> >
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel