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

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

Hi Leif,
Would you like to recheck the new patches I sent?

Thanks
Abner

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist)
> Sent: Friday, January 29, 2021 1:11 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
> 
> 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