Re: [PATCH 2/3] bitset: fix build for GCC without experimental API

[Thomas Monjalon <thomas@monjalon.net>]

15/10/2024 16:44, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Tuesday, 15 October 2024 16.13
> > 
> > 15/10/2024 14:53, Morten Brørup:
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > @@ -255,7 +255,13 @@ __rte_experimental
> > > >  static inline bool
> > > >  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> > > >  {
> > > > +#ifdef ALLOW_EXPERIMENTAL_API
> > > >  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > > > +#else
> > > > +	RTE_SET_USED(bitset);
> > > > +	RTE_SET_USED(bit_num);
> > > > +	return false;
> > > > +#endif
> > > >  }
> > >
> > > This looks wrong! The API is exposed, but does nothing.
> > 
> > Yes, this is what we did already in the past for other experimental
> > functions
> > called from inline functions.
> > There is no choice, the compiler is hitting the warning.
> > 
> > > It is possible to build without ALLOW_EXPERIMENTAL_API; the compiler
> > will emit warnings when using experimental APIs.
> > 
> > Yes it is possible to build,
> > but it is not said it should work the same.
> > 
> > > If those compiler warnings are not handled as errors, the compiled
> > application will be full of bugs.
> > 
> > Yes, that's why there are warnings.
> > We may document it better but that's the behavior we have for years.
> > There is no easy solution, and making experimental functions work
> > without defining ALLOW_EXPERIMENTAL_API is not a really interesting
> > goal.
> > I think the word "allow" suggests it is not supposed to work if not
> > allowed.
> 
> There's a world of difference between "experimental, might have bugs" - which is what I (and possibly other DPDK consumers) expect - and "experimental, we know for a fact that it doesn't work" - which is quite a surprise to me.

It does not work if you don't enable it,
and there is a warning when compiling.


> > It would be more interesting to make sure the users understand
> > why we have this flag and how to enable it.
> > I propose adding some docs, and mentioning ALLOW_EXPERIMENTAL_API
> > in the the __rte_experimental message in rte_compat.h.
> > 
> 
> If we know that some of these warnings cause bugs in DPDK, we should elevate these specific instances to error level.

Technically I don't know whether it is possible.
Look at rte_compat.h


> Regarding this specific patch:
> Would it be possible to change it to behave like patch 1/3, i.e. completely omit the experimental APIs if ALLOW_EXPERIMENTAL_API is not defined?

I disagree, it would be a lot of churn in the code to disable all experimental API calls.