Re: [edk2-devel] [edk2-platforms][PATCH v1 1/2] MinPlatformPkg/BoardAcpiEnableLibNull: Improve maintainability

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Michael,

Feedback is line.

Thanks,
Nate

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Wednesday, April 7, 2021 11:33 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/2]
> MinPlatformPkg/BoardAcpiEnableLibNull: Improve maintainability
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> The NULL instance of BoardAcpiEnableLib in MinPlatformPkg currently has a
> few organization issues that make it more difficult to find and use than a
> typical NULL library instance.
> 
> 1. It shares a directory with another unrelated library instance.
> 2. The directory name "BoardAcpiLibNull" is not directly related to
>    either library instance name in the directory.
> 3. The library instance has unnecessary dependencies.
> 4. The BASE_NAME does not indicate the library instance is the NULL
>    instance.
> 5. The C source file name does not match the INF file name making
>    finding the C source by search more cumbersome than needed.
> 
> This change resolves the above issues to improve maintainability.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull/BoardAcpiEn
> ableLib.c => BoardAcpiEnableLibNull/BoardAcpiEnableLibNull.c} |  4 +---
>  Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull =>
> BoardAcpiEnableLibNull}/BoardAcpiEnableLibNull.inf                    | 12 ++++------
> --
>  Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> |  4 ++--
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLib.c
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Boar
> dAcpiEnableLibNull.c
> similarity index 73%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEna
> bleLib.c
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Board
> AcpiEnableLibNull.c
> index 334cb21714ac..494389ae5dc8 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLib.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> +++ BoardAcpiEnableLibNull.c
> @@ -5,10 +5,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
> -#include <Library/BoardAcpiTableLib.h>
> +#include <Base.h>

Any reason for choosing Base.h? From what I see this file uses types that require Uefi/UefiBaseType.h like EFI_STATUS for example. It will still compile fine since BoardAcpiEnableLib.h includes Uefi.h (which includes Uefi/UefiBaseType.h). So I wonder what the value of including only Base.h is here since the file will still have implicit dependencies.

>  #include <Library/BoardAcpiEnableLib.h> -#include <Library/PcdLib.h> -
> #include <Library/DebugLib.h>
> 
>  EFI_STATUS
>  EFIAPI
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLibNull.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Boar
> dAcpiEnableLibNull.inf
> similarity index 67%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEna
> bleLibNull.inf
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Board
> AcpiEnableLibNull.inf
> index 54985ee50d5f..90bed3760e2a 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLibNull.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> +++ BoardAcpiEnableLibNull.inf
> @@ -1,7 +1,8 @@
>  ## @file
> -# Component information file for Board Acpi Library
> +# Component information file for NULL instance of the Board ACPI Enable
> +library
>  #
>  # Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) Microsoft Corporation.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -9,20 +10,15 @@
> 
>  [Defines]
>    INF_VERSION                    = 0x00010005
> -  BASE_NAME                      = BoardAcpiEnableLib
> +  BASE_NAME                      = BoardAcpiEnableLibNull
>    FILE_GUID                      = EAF8AA6D-549A-40CC-8FB2-3A1FFEC41601
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = BoardAcpiEnableLib
> 
> -[LibraryClasses]
> -  BaseLib
> -  PcdLib
> -  DebugLib
> -
>  [Packages]
>    MinPlatformPkg/MinPlatformPkg.dec
>    MdePkg/MdePkg.dec
> 
>  [Sources]
> -  BoardAcpiEnableLib.c
> +  BoardAcpiEnableLibNull.c
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> index 998ee7909568..da27aa1c4227 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> @@ -79,7 +79,7 @@ [LibraryClasses.common]
> 
> 
> BoardInitLib|MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInit
> LibNull.inf
> 
> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAc
> piTableLibNull.inf
> -
> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardA
> cpiEnableLibNull.inf
> +
> +
> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> + BoardAcpiEnableLibNull.inf
> 
> SiliconPolicyInitLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibN
> ull/SiliconPolicyInitLibNull.inf
> 
> SiliconPolicyUpdateLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyUp
> dateLibNull/SiliconPolicyUpdateLibNull.inf
> 
> @@ -150,7 +150,7 @@ [Components]
>    MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>    MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>    MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
> -  MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEnableLibNull.inf
> +
> +
> MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibN
> + ull.inf
>    MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTableLibNull.inf
> 
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpi
> SupportLib.inf
> 
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAc
> piSupportLib.inf
> --
> 2.28.0.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#73791): https://edk2.groups.io/g/devel/message/73791
> Mute This Topic: https://groups.io/mt/81923537/1767664
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [nathaniel.l.desimone@intel.com]
> -=-=-=-=-=-=
>