[edk2-devel] [PATCH] RedfishPkg/JsonLib: Fix build errors

[Abner Chang]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif at nuviainc.com]
> Sent: Thursday, January 28, 2021 7:17 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>
> Cc: devel at edk2.groups.io; Wang, Nickle (HPS SW) <nickle.wang at hpe.com>;
> Michael D Kinney <michael.d.kinney at intel.com>
> Subject: Re: [edk2-devel] [PATCH] RedfishPkg/JsonLib: Fix build errors
> 
> On Thu, Jan 28, 2021 at 04:02:54 +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> > > > +  #   C4706: assignment within conditional expression
> > > >    #
> > > >    # Define macro HAVE_CONFIG_H to include
> > > > jansson_private_config.h to
> > > build.
> > > >    # Undefined _WIN32, WIN64, _MSC_VER macros
> > > >    # On GCC, no error on the unused-function and unused-but-set-
> variable.
> > > >    #
> > > > -  MSFT:*_*_X64_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4334
> > > > /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > > > -  MSFT:*_*_IA32_CC_FLAGS = /wd4204 /wd4244 /wd4090
> > > /DHAVE_CONFIG_H=1
> > > > /U_WIN32 /UWIN64 /U_MSC_VER
> > > > +  MSFT:*_*_X64_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4334
> > > /wd4706
> > > > + /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > > > + MSFT:*_*_IA32_CC_FLAGS = /wd4204 /wd4244 /wd4090 /wd4706
> > > > + /DHAVE_CONFIG_H=1 /U_WIN32 /UWIN64 /U_MSC_VER
> > >
> > > Urgh, please don't do this.
> > > C4706 is what gives you a warning for accidentally assigning instead
> > > of comparing. It's our last defence against the jeopardy-comparing
> > > hordes that think
> > >   if (NULL == Ptr)
> > > is a sensible way of writing C.
> > >
> > > What is the actual problem being encountered?
> >
> > That is because we use the macro defined in open source header file,
> > RedfishPkg/Library/JsonLib/jansson/src/jansson.h
> >
> > #define json_object_foreach(object, key, value)                                          \
> >     for (key = json_object_iter_key(json_object_iter(object));                           \
> >          key && (value =
> json_object_iter_value(json_object_key_to_iter(key)));          \
> >          key = json_object_iter_key(                                                     \
> >              json_object_iter_next(object,
> > json_object_key_to_iter(key))))
> 
> Yay, "optimisation" by using preprocessor...
> 
> Apart from how this ought to be a helper function:
> - No parentheses around parameters in macro.
> - Setting "value" at each iteration through the loop condition.
> - Slinging parentheses like it's a lisp convention.
>   - And it would be worse if they treated the parameters properly.
> 
> If we ignore the other issues, the below should be functionally equivalent
> and build on VS without disabling the warning:
> 
>   for (key = json_object_iter_key(json_object_iter(object));                           \
>        key;                                                                            \
>        key = json_object_iter_key(                                                     \
>             json_object_iter_next(object, json_object_key_to_iter(key))))
> {            \
>     value = json_object_iter_value(json_object_key_to_iter(key));
> \
>     if (!value)                                                                        \
>       break;                                                                           \
>   }
> 
> Could you try to convince upstream to take the patch?
Sure, I just sent an email to community, hope we can get the feedback soon.

Leif, that may takes time to have this patch to be in upstream (if they agree with it)... I have another PR which is not get merged yet.
This will block our works on edk2, thus I would like to just add build option to suppress this build error. The build option is only for JsonLib though. Do you agree with this?

Thanks
Abner

> 
> > > Beyond that, this will probably be an issue for all architectures,
> > > so why add explicit (identical) workarounds for IA32/X64?
> >
> > I didn't catch this build error on GCC. You may know why?
> 
> Ugh.
> This is because (for whatever reason), both clang and GCC suppress this
> warning if you add parentheses around the assignment. VS does not.
> 
> /
>     Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70852): https://edk2.groups.io/g/devel/message/70852
Mute This Topic: https://groups.io/mt/80097450/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-