Re: [edk2-devel] [PATCH V1 2/2] MdeModulePkg PeiCore: Improve comment semantics

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Hi Philippe,

Thanks for your suggestions. I incorporated some of the suggestions into the V2 series.

Thanks,
Michael

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Philippe
> Mathieu-Daudé
> Sent: Wednesday, November 27, 2019 3:49 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH V1 2/2] MdeModulePkg PeiCore: Improve
> comment semantics
> 
> On 11/27/19 5:06 AM, Kubacki, Michael A via Groups.Io wrote:
> > Clarifies wording in several PeiCore comments to improve
> 
> "Clarify"?
>

 
I believe an implied subject noun led to some confusion so I just explicitly included that in the V2 message.

> > reading comprehension.
> >
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > ---
> >   MdeModulePkg/Core/Pei/FwVol/FwVol.h           |  4 ++--
> >   MdeModulePkg/Core/Pei/PeiMain.h               | 11 +++++-----
> >   MdeModulePkg/Core/Pei/Dependency/Dependency.c |  4 ++--
> >   MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c |  4 ++--
> >   MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 23 ++++++++++----------
> >   MdeModulePkg/Core/Pei/Memory/MemoryServices.c |  4 ++--
> >   6 files changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > b/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > index 263f0d7a56..8aaf84870b 100644
> > --- a/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > +++ b/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > @@ -303,9 +303,9 @@ FindFileEx (
> >     );
> >
> >   /**
> > -  Report the information for a new discovered FV in unknown format.
> > +  Report the information for a newly discovered FV in an unknown format.
> >
> > -  If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for
> > specific FV format, but
> > +  If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for a
> > + third-party FV format, but
> >     the FV in this FV format has been discovered, then the information of
> this FV
> >     will be cached into PEI_CORE_INSTANCE's UnknownFvInfo array.
> >     Also a notification would be installed for unknown FV format GUID,
> > if EFI_PEI_FIRMWARE_VOLUME_PPI diff --git
> > a/MdeModulePkg/Core/Pei/PeiMain.h
> b/MdeModulePkg/Core/Pei/PeiMain.h
> > index 3f61247a0f..96d6df0485 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain.h
> > +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> > @@ -1217,8 +1217,8 @@ PeiFfsGetVolumeInfo (
> >     );
> >
> >   /**
> > -  This routine enable a PEIM to register itself to shadow when PEI
> > Foundation
> > -  discovery permanent memory.
> > +  This routine enables a PEIM to register itself for shadow when the
> > + PEI Foundation  discovers permanent memory.
> >
> >     @param FileHandle             File handle of a PEIM.
> >
> > @@ -1314,12 +1314,13 @@ ProcessFvFile (
> >     );
> >
> >   /**
> > -  Get instance of PEI_CORE_FV_HANDLE for next volume according to
> given index.
> > +  Gets a PEI_CORE_FV_HANDLE instance for the next volume according to
> the given index.
> 
> "Get"?
> 

I left as-is in V2. You can see similar usage in the majority of function descriptions in BaseLib.h for example.

> >
> > -  This routine also will install FvInfo PPI for FV HOB in PI ways.
> > +  This routine also will install an instance of the FvInfo PPI for
> > + the FV HOB  as defined in the PI specification.
> >
> >     @param Private    Pointer of PEI_CORE_INSTANCE
> > -  @param Instance   The index of FV want to be searched.
> > +  @param Instance   The index of the FV to search.
> 
> Maybe without "The" and trailing dot?
> 

Done in V2.

> >
> >     @return Instance of PEI_CORE_FV_HANDLE.
> >   **/
> > diff --git a/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > b/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > index 9a8353aef2..b53e5f2686 100644
> > --- a/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > +++ b/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > @@ -2,8 +2,8 @@
> >     PEI Dispatcher Dependency Evaluator
> >
> >     This routine evaluates a dependency expression
> > (DEPENDENCY_EXPRESSION) to determine
> > -  if a driver can be scheduled for execution.  The criteria for
> > -  schedulability is that the dependency expression is satisfied.
> > +  if a driver can be scheduled for execution.  The criteria to be
> > + scheduled is  that the dependency expression is satisfied.
> >
> >   Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> >   SPDX-License-Identifier: BSD-2-Clause-Patent diff --git
> > a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > index c9f2a91264..a18ac47f61 100644
> > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > @@ -1347,8 +1347,8 @@ DepexSatisfied (
> >   }
> >
> >   /**
> > -  This routine enable a PEIM to register itself to shadow when PEI
> > Foundation
> > -  discovery permanent memory.
> > +  This routine enables a PEIM to register itself for shadow when the
> > + PEI Foundation  discovers permanent memory.
> >
> >     @param FileHandle             File handle of a PEIM.
> >
> > diff --git a/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > b/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > index c21eb9c039..a9fa476846 100644
> > --- a/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > +++ b/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > @@ -593,7 +593,7 @@ FirmwareVolumeInfoPpiNotifyCallback (
> >     }
> >
> >     //
> > -  // Locate the corresponding FV_PPI according to founded FV's format
> > GUID
> > +  // Locate the corresponding FV_PPI according to the format GUID of
> > + the FV found
> >     //
> >     Status = PeiServicesLocatePpi (
> >                &FvInfo2Ppi.FvFormat,
> > @@ -1533,7 +1533,7 @@ ProcessFvFile (
> >         );
> >
> >       //
> > -    // Inform the extracted FvImage to FV HOB consumer phase, i.e. DXE
> phase
> > +    // Expose the extracted FvImage to the FV HOB consumer phase,
> > + i.e. DXE phase
> >       //
> >       BuildFvHob (
> >         (EFI_PHYSICAL_ADDRESS) (UINTN) FvHeader, @@ -2087,12 +2087,13
> > @@ FvHandleToCoreHandle (
> >   }
> >
> >   /**
> > -  Get instance of PEI_CORE_FV_HANDLE for next volume according to
> given index.
> > +  Gets a PEI_CORE_FV_HANDLE instance for the next volume according to
> the given index.
> 
> "Get"?
>

As in FwVol.h, I left as-is in V2.

> >
> > -  This routine also will install FvInfo PPI for FV HOB in PI ways.
> > +  This routine also will install an instance of the FvInfo PPI for
> > + the FV HOB  as defined in the PI specification.
> >
> >     @param Private    Pointer of PEI_CORE_INSTANCE
> > -  @param Instance   The index of FV want to be searched.
> > +  @param Instance   The index of the FV to search.
> 
> Without "The"/trailing dot?
> 

As in FwVol.h, this is done in V2.

> >
> >     @return Instance of PEI_CORE_FV_HANDLE.
> >   **/
> > @@ -2185,13 +2186,13 @@ PeiReinitializeFv (
> >   }
> >
> >   /**
> > -  Report the information for a new discovered FV in unknown third-party
> format.
> > +  Report the information for a newly discovered FV in an unknown format.
> >
> > -  If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for
> > third-party FV format, but
> > -  the FV in this format has been discovered, then this FV's
> > information will be cached into
> > -  PEI_CORE_INSTANCE's UnknownFvInfo array.
> > -  Also a notification would be installed for unknown third-party FV
> > format guid, if EFI_PEI_FIRMWARE_VOLUME_PPI
> > -  is installed later by platform's PEIM, the original unknown
> > third-party FV will be processed by
> > +  If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for a
> > + third-party FV format, but  the FV in this FV format has been
> > + discovered, then the information of this FV
> 
> Maybe "in this FV format" is redundant?
> 

Removed "in this FV format" in V2.

> > +  will be cached into PEI_CORE_INSTANCE's UnknownFvInfo array.
> > +  Also a notification would be installed for unknown FV format GUID,
> > + if EFI_PEI_FIRMWARE_VOLUME_PPI  is installed later by platform's
> > + PEIM, the original unknown FV will be processed by
> >     using new installed EFI_PEI_FIRMWARE_VOLUME_PPI.
> >
> >     @param PrivateData  Point to instance of PEI_CORE_INSTANCE diff
> > --git a/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > b/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > index 838a003baa..e713e6811a 100644
> > --- a/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > +++ b/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > @@ -759,7 +759,7 @@ PeiFreePages (
> >   /**
> >
> >     Pool allocation service. Before permanent memory is discovered,
> > the pool will
> > -  be allocated the heap in the temporary memory. Generally, the size
> > of heap in temporary
> > +  be allocated in the heap in temporary memory. Generally, the size
> > + of the heap in temporary
> >     memory does not exceed to 64K, so the biggest pool size could be
> allocated is
> >     64K.
> >
> > @@ -789,7 +789,7 @@ PeiAllocatePool (
> >     //
> >
> >     //
> > -  // Generally, the size of heap in temporary memory does not exceed
> > to 64K,
> > +  // Generally, the size of heap in temporary memory does not exceed
> > + 64K,
> >     // HobLength is multiples of 8 bytes, so the maximum size of pool is
> 0xFFF8 - sizeof (EFI_HOB_MEMORY_POOL)
> >     //
> >     if (Size > (0xFFF8 - sizeof (EFI_HOB_MEMORY_POOL))) {
> >
> 
> 
>