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

["Leif Lindholm" <leif@nuviainc.com>]

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?

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