03/09/2018 11:29, Adrien Mazarguil:
> Hi Christian,
>
> Couldn't follow up on this last week, however I still have some concerns and
> comments, please see below.
>
> On Thu, Aug 30, 2018 at 01:59:59PM +0200, Christian Ehrhardt wrote:
> > The definition of almost any newer standard like --stc=c11 will drop
> > __APPLCE_ALTIVEC__ which otherwise would be defined.
> > If that is the case then altivec.h will redefine bool to a type
> > conflicting with those defined by stdbool.h.
> >
> > This breaks compilation of 18.08 on ppc64 like:
> > mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> > to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> > from type ‘int’ in_port_id_set = false;
> >
> > Other alternatives were pursued on [1] but they always ended up being
> > more complex than what would be appropriate for the issue we face.
> >
> > [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> >
> > Tested-by: Takeshi T Yoshimura <t...@jp.ibm.com>
> > Reviewed-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> > Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
> > ---
> > .../common/include/arch/ppc_64/rte_memcpy.h | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > @@ -37,6 +37,17 @@
> > #include <string.h>
> > /*To include altivec.h, GCC version must >= 4.8 */
> > #include <altivec.h>
> > +/*
> > + * Compilation workaround for PPC64 targets when AltiVec is fully
> > + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> > + * of "bool" between stdbool and altivec.
> > + */
> > +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
> > + #undef bool
> > + /* redefine as in stdbool.h */
> > + #define bool _Bool
> > +#endif
> > +
>
> The above will break existing C++ programs that include rte_memcpy.h.
>
> Problem is that bool is an actual C++ type. C99 has _Bool which doesn't
> exist in C++ along with a bool macro that appears only after including
> stdbool.h.
>
> To make things worse, nothing prevents C++ programs from importing a C-style
> bool macro by including stdbool.h (or cstdbool).
>
> Enclosing it in #ifdef __cplusplus won't help because you never know what
> bool is supposed to be in the first place as it depends on how applications
> are written. I think something like this prior suggestion [1]
> (saving/restoring bool) is the only way to deal with that in a safe-ish
> fashion.
>
> Pending something better, the above #undef/#define workaround is only safe
> to use inside mlx5 PMD code that triggers the compilation issue. It must not
> be found in a public header.
>
> > #ifdef __cplusplus
> > extern "C" {
>
> [1] https://mails.dpdk.org/archives/dev/2018-August/110401.html
Thank you for the review Adrien. I think we could accept the patch
if some notes mention it breaks C++ applications.
But...
After 2 months, nobody replied or complained about the issue not fixed.
So I classify this patch as rejected.
Summary of alerts here:
http://mails.dpdk.org/archives/dev/2018-November/118259.html
