Re: [PATCH v3 2/7] ShellPkg/AcpiView: Refactor configuration

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

See below.

> -----Original Message-----
> From: Tomas Pilar <Tomas.Pilar@arm.com>
> Sent: Monday, June 15, 2020 10:04 PM
> To: devel@edk2.groups.io
> Cc: nd@arm.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: [PATCH v3 2/7] ShellPkg/AcpiView: Refactor configuration
> 
> A new file and header (AcpiViewConfig.[ch]) is created that houses the user
> configuration. This declutters the core code and improves modularity of the
> design.
> 
> The module level symbols for verbosity, table selection, and highlighting are
> refactored into the new file.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Tomas Pilar <tomas.pilar@arm.com>
> ---
>  .../UefiShellAcpiViewCommandLib/AcpiParser.c  |   1 +
>  .../AcpiTableParser.c                         |   1 +
>  .../UefiShellAcpiViewCommandLib/AcpiView.c    | 237 +++--------------
>  .../UefiShellAcpiViewCommandLib/AcpiView.h    |  95 -------
>  .../AcpiViewConfig.c                          | 246 ++++++++++++++++++
>  .../AcpiViewConfig.h                          | 177 +++++++++++++
>  .../Parsers/Gtdt/GtdtParser.c                 |   1 +
>  .../Parsers/Iort/IortParser.c                 |   1 +
>  .../Parsers/Madt/MadtParser.c                 |   1 +
>  .../Parsers/Pptt/PpttParser.c                 |   1 +
>  .../Parsers/Srat/SratParser.c                 |   1 +
>  .../UefiShellAcpiViewCommandLib.inf           |  32 +--
>  12 files changed, 479 insertions(+), 315 deletions(-)  create mode 100644
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.c
>  create mode 100644
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewConfig.h
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index 3f12a33050a4..02f6d771c7e1 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -10,6 +10,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiView.h"
> +#include "AcpiViewConfig.h"
> 
>  STATIC UINT32   gIndent;
>  STATIC UINT32   mTableErrorCount;
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> index d5b9eee52323..4b618f131eac 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.c
> @@ -17,6 +17,7 @@
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
>  #include "AcpiView.h"
> +#include "AcpiViewConfig.h"
> 
>  #if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)  #include
> "Arm/SbbrValidator.h"
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
> index f1a95b7b8f03..390e378e9a6c 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiView.c
> @@ -20,6 +20,7 @@
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
>  #include "AcpiView.h"
> +#include "AcpiViewConfig.h"
>  #include "UefiShellAcpiViewCommandLib.h"
> 
>  #if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) @@ -28,17 +29,8
> @@
> 
>  EFI_HII_HANDLE gShellAcpiViewHiiHandle = NULL;
> 
> -// Report variables
> -STATIC UINT32             mSelectedAcpiTable;
> -STATIC CONST CHAR16*      mSelectedAcpiTableName;
> -STATIC BOOLEAN            mSelectedAcpiTableFound;
> -STATIC EREPORT_OPTION     mReportType;
>  STATIC UINT32             mTableCount;
>  STATIC UINT32             mBinTableCount;
> -STATIC BOOLEAN            mConsistencyCheck;
> -STATIC BOOLEAN            mColourHighlighting;
> -STATIC BOOLEAN            mMandatoryTableValidate;
> -STATIC UINTN              mMandatoryTableSpec;
> 
>  /**
>    An array of acpiview command line parameters.
> @@ -53,142 +45,6 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
>    {NULL, TypeMax}
>  };
> 
> -/**
> -  This function returns the colour highlighting status.
> -
> -  @retval TRUE if colour highlighting is enabled.
> -**/
> -BOOLEAN
> -GetColourHighlighting (
> -  VOID
> -  )
> -{
> -  return mColourHighlighting;
> -}
> -
> -/**
> -  This function sets the colour highlighting status.
> -
> -  @param  Highlight       The Highlight status.
> -
> -**/
> -VOID
> -SetColourHighlighting (
> -  BOOLEAN Highlight
> -  )
> -{
> -  mColourHighlighting = Highlight;
> -}
> -
> -/**
> -  This function returns the consistency checking status.
> -
> -  @retval TRUE if consistency checking is enabled.
> -**/
> -BOOLEAN
> -GetConsistencyChecking (
> -  VOID
> -  )
> -{
> -  return mConsistencyCheck;
> -}
> -
> -/**
> -  This function sets the consistency checking status.
> -
> -  @param  ConsistencyChecking   The consistency checking status.
> -
> -**/
> -VOID
> -SetConsistencyChecking (
> -  BOOLEAN ConsistencyChecking
> -  )
> -{
> -  mConsistencyCheck = ConsistencyChecking; -}
> -
> -/**
> -  This function returns the ACPI table requirements validation flag.
> -
> -  @retval TRUE if check for mandatory table presence should be performed.
> -**/
> -BOOLEAN
> -GetMandatoryTableValidate (
> -  VOID
> -  )
> -{
> -  return mMandatoryTableValidate;
> -}
> -
> -/**
> -  This function sets the ACPI table requirements validation flag.
> -
> -  @param  Validate    Enable/Disable ACPI table requirements validation.
> -**/
> -VOID
> -SetMandatoryTableValidate (
> -  BOOLEAN Validate
> -  )
> -{
> -  mMandatoryTableValidate = Validate;
> -}
> -
> -/**
> -  This function returns the identifier of specification to validate ACPI table
> -  requirements against.
> -
> -  @return   ID of specification listing mandatory tables.
> -**/
> -UINTN
> -GetMandatoryTableSpec (
> -  VOID
> -  )
> -{
> -  return mMandatoryTableSpec;
> -}
> -
> -/**
> -  This function sets the identifier of specification to validate ACPI table
> -  requirements against.
> -
> -  @param  Spec      ID of specification listing mandatory tables.
> -**/
> -VOID
> -SetMandatoryTableSpec (
> -  UINTN Spec
> -  )
> -{
> -  mMandatoryTableSpec = Spec;
> -}
> -
> -/**
> -  This function returns the report options.
> -
> -  @retval Returns the report option.
> -**/
> -STATIC
> -EREPORT_OPTION
> -GetReportOption (
> -  VOID
> -  )
> -{
> -  return mReportType;
> -}
> -
> -/**
> -  This function returns the selected ACPI table.
> -
> -  @retval Returns signature of the selected ACPI table.
> -**/
> -STATIC
> -UINT32
> -GetSelectedAcpiTable (
> -  VOID
> -  )
> -{
> -  return mSelectedAcpiTable;
> -}
> -
>  /**
>    This function dumps the ACPI table to a file.
> 
> @@ -205,19 +61,21 @@ DumpAcpiTableToFile (
>    IN CONST UINTN   Length
>    )
>  {
> -  EFI_STATUS         Status;
> -  CHAR16             FileNameBuffer[MAX_FILE_NAME_LEN];
> -  SHELL_FILE_HANDLE  DumpFileHandle;
> -  UINTN              TransferBytes;
> +  EFI_STATUS          Status;
> +  CHAR16              FileNameBuffer[MAX_FILE_NAME_LEN];
> +  SHELL_FILE_HANDLE   DumpFileHandle;
> +  UINTN               TransferBytes;
> +  SELECTED_ACPI_TABLE *SelectedTable;
> 
>    DumpFileHandle = NULL;
>    TransferBytes = Length;
> +  GetSelectedAcpiTable (&SelectedTable);
> 
>    UnicodeSPrint (
>      FileNameBuffer,
>      sizeof (FileNameBuffer),
>      L".\\%s%04d.bin",
> -    mSelectedAcpiTableName,
> +    SelectedTable->Name,
>      mBinTableCount++
>      );
> 
> @@ -273,10 +131,11 @@ ProcessTableReportOptions (
>    IN CONST UINT32  Length
>    )
>  {
> -  UINTN   OriginalAttribute;
> -  UINT8*  SignaturePtr;
> -  BOOLEAN Log;
> -  BOOLEAN HighLight;
> +  UINTN                OriginalAttribute;
> +  UINT8               *SignaturePtr;
> +  BOOLEAN              Log;
> +  BOOLEAN              HighLight;
> +  SELECTED_ACPI_TABLE *SelectedTable;

Make the *SignaturePtr & *SelectedTable align with other variable. Except this, Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao