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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, 15 October 2024 21.58
> 
> 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.

My issue with this is:
The warning only says that the API is deprecated. It doesn't say that the underlying implementation might be completely missing.

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

OK.
Apparently the issue is not specific to this patch, so it would be unreasonable to require this patch to handle it differently than it has been handled until now.

Acked-by: Morten Brørup <mb@smartsharesystems.com>