Re: [staging/dynamictables PATCH v1 4/5] DynamicTablesPkg: Fix macro to prevent side effect

[Sami Mujawar <Sami.Mujawar@arm.com>]

Hi Leif,

I agree it will be good to update the code to use ALIGN_VALUE(x,4).

I can see 2 options to fix this:
  - Resubmit a v2 patchset with this change.
OR
  - I can submit a separate patch (on top of the current patchset) for this change.

Can you let me know which one is preferred, please?

Regards,

Sami Mujawar
-----Original Message-----
From: Leif Lindholm <leif.lindholm@linaro.org> 
Sent: 28 June 2018 05:51 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: edk2-devel@lists.01.org; Evan Lloyd <Evan.Lloyd@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: Re: [staging/dynamictables PATCH v1 4/5] DynamicTablesPkg: Fix macro to prevent side effect

On Wed, Jun 27, 2018 at 05:47:45PM +0100, Sami Mujawar wrote:
> The ALIGN32 macro had undesired side effects when used with binary 
> operators and was generating compilation errors.
> 
> Added brackets to fix this issue.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Fix macro side effect                                         [SAMI]
> 
>  DynamicTablesPkg/Include/Library/TableHelperLib.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/DynamicTablesPkg/Include/Library/TableHelperLib.h 
> b/DynamicTablesPkg/Include/Library/TableHelperLib.h
> index 
> b358223434af76820d34c29c67325919a2283aa7..3eac28a158e9621f6f33a5b53896
> 4a16e6b69112 100644
> --- a/DynamicTablesPkg/Include/Library/TableHelperLib.h
> +++ b/DynamicTablesPkg/Include/Library/TableHelperLib.h
> @@ -17,7 +17,7 @@
>  
>  /** A helper macro to align a value to the 32-bit word boundary  */ 
> -#define ALIGN32(x) ((x) + (sizeof (UINT32) - 1)) & ~(sizeof (UINT32) 
> - 1)
> +#define ALIGN32(x) (((x) + (sizeof (UINT32) - 1)) & ~(sizeof (UINT32) 
> +- 1))

Not as such a comment on this patch, but this problem could also be resolved by dropping this local macro in favour of the Base.h ALIGN_VALUE(x, 4) ... or just redefining it to use that.

/
    Leif

>  
>  /** The GetCgfMgrInfo function gets the CM_STD_OBJ_CONFIGURATION_MANAGER_INFO
>      object from the Configuration Manager.
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
>