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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Thanks for the feedback. I removed Base.h in a v2 series just sent.


On 4/7/2021 5:29 PM, Nate DeSimone wrote:
> 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
> 
> 
> 
> 
>