Re: [edk2] [staging/dynamictables PATCH v1 4/5] DynamicTablesPkg: Fix macro to prevent side effect
On Thu, Jun 28, 2018 at 05:10:48PM +, 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 > Sent: 28 June 2018 05:51 PM > To: Sami Mujawar > Cc: edk2-devel@lists.01.org; Evan Lloyd ; Matteo Carlini > ; Stephanie Hughes-Fitt > ; nd > 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 > > --- > > > > 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)' > > > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [staging/dynamictables PATCH v1 4/5] DynamicTablesPkg: Fix macro to prevent side effect
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 Sent: 28 June 2018 05:51 PM To: Sami Mujawar Cc: edk2-devel@lists.01.org; Evan Lloyd ; Matteo Carlini ; Stephanie Hughes-Fitt ; nd 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 > --- > > 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)' > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [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 > --- > > 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..3eac28a158e9621f6f33a5b538964a16e6b69112 > 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)' > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
