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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Jun 28, 2018 at 05:10:48PM +0000, Sami Mujawar wrote:
> 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?

It won't actually garble the history to do it as a separate patch, so
I'm happy with that.

But I'm still waiting on a clarification from Evan.

Best Regards,

Leif

> 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)'
> > 
> >