Re: [edk2-platforms][PATCH v1 2/2] MinPlatformPkg/BoardAcpiTableLibNull: Improve maintainability

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

Hi Michael,

Feedback is line.

Thanks,
Nate

> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> 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-platforms][PATCH v1 2/2]
> MinPlatformPkg/BoardAcpiTableLibNull: Improve maintainability
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> The NULL instance of BoardAcpiTableLib 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 use and 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/BoardAcpiTa
> bleLib.c => BoardAcpiTableLibNull/BoardAcpiTableLibNull.c} |  4 +---
>  Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull =>
> BoardAcpiTableLibNull}/BoardAcpiTableLibNull.inf                   | 12 ++++--------
>  Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> |  4 ++--
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT
> ableLib.c
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Board
> AcpiTableLibNull.c
> similarity index 73%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTab
> leLib.c
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAc
> piTableLibNull.c
> index e49e6ad44162..0f871b6f07ef 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT
> ableLib.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/B
> +++ oardAcpiTableLibNull.c
> @@ -5,10 +5,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
> +#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/BoardAcpiTableLib.h>
> -#include <Library/BoardAcpiEnableLib.h> -#include <Library/PcdLib.h> -
> #include <Library/DebugLib.h>
> 
>  EFI_STATUS
>  EFIAPI
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT
> ableLibNull.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Board
> AcpiTableLibNull.inf
> similarity index 67%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTab
> leLibNull.inf
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAc
> piTableLibNull.inf
> index 04f55b49d5a1..6102897ab67b 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT
> ableLibNull.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/B
> +++ oardAcpiTableLibNull.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                      = BoardAcpiTableLib
> +  BASE_NAME                      = BoardAcpiTableLibNull
>    FILE_GUID                      = F220FAB7-F8E4-4E7A-A599-D47E2D547956
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = BoardAcpiTableLib
> 
> -[LibraryClasses]
> -  BaseLib
> -  PcdLib
> -  DebugLib
> -
>  [Packages]
>    MinPlatformPkg/MinPlatformPkg.dec
>    MdePkg/MdePkg.dec
> 
>  [Sources]
> -  BoardAcpiTableLib.c
> +  BoardAcpiTableLibNull.c
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> index da27aa1c4227..cf3ff13e7b29 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> @@ -78,7 +78,7 @@ [LibraryClasses.common]
> 
> FspWrapperPlatformLib|MinPlatformPkg/FspWrapper/Library/PeiFspWrapp
> erPlatformLib/PeiFspWrapperPlatformLib.inf
> 
> 
> BoardInitLib|MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInit
> LibNull.inf
> -
> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAc
> piTableLibNull.inf
> +
> +
> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Bo
> + ardAcpiTableLibNull.inf
> 
> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/B
> oardAcpiEnableLibNull.inf
> 
> SiliconPolicyInitLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibN
> ull/SiliconPolicyInitLibNull.inf
> 
> SiliconPolicyUpdateLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyUp
> dateLibNull/SiliconPolicyUpdateLibNull.inf
> @@ -151,7 +151,7 @@ [Components]
>    MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>    MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
> 
> MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibN
> ull.inf
> -  MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTableLibNull.inf
> +
> + MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAcpiTableLibNul
> + l.inf
> 
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpi
> SupportLib.inf
> 
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAc
> piSupportLib.inf
> 
> --
> 2.28.0.windows.1