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

["Abner Chang" <abner.chang@hpe.com>]

Hi Leif,
I split this patch into 3 separate patches. Please ignore the old one.

Thank
Abner

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist)
> Sent: Thursday, January 28, 2021 10:31 PM
> To: Leif Lindholm <leif@nuviainc.com>
> Cc: devel@edk2.groups.io; Wang, Nickle (HPS SW) <nickle.wang@hpe.com>;
> Michael D Kinney <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH] RedfishPkg/JsonLib: Fix build errors
> 
> 
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > Sent: Thursday, January 28, 2021 7:17 PM
> > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> > Cc: devel@edk2.groups.io; Wang, Nickle (HPS SW)
> <nickle.wang@hpe.com>;
> > Michael D Kinney <michael.d.kinney@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