RE: [PATCH v5 0/3] add portable version of __builtin_add_overflow

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

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 4 June 2025 12.41
> 
> On Wed, Jun 4, 2025 at 12:29 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > I am not a fan of adding such public API, an internal API would be
> > > enough.
> > > Do you plan to add more helpers for math operations?
> > >
> > > For the current helper, the only user is a driver (base code).
> > > Can't we just wrap a __builtin_add_overflow (under #ifdef msvc) in
> the
> > > osdep.h header?
> >
> > We already have public APIs for bit operations in rte_bitops.h.
> > This math API follows the same principle; and math operations - just
> like bit operations - might be useful for DPDK applications, so let's
> keep it public.
> 
> This comparison is poor.
> 
> There are many users of bitops in dpdk, and *public* headers needed it.

I don't think the number of uses of a generic function should determine if it should be public or private.
The important thing is avoiding copy-pasting.

> 
> Here, we have one single function in a driver implementation.
> And this code is unused (__builtin_add_overflow -> check_add_overflow
> -> ice_get_pfa_module_tlv -> ice_get_link_default_override ->
> ice_cfg_phy_fec, with no intree user).
> 

I'm mainly saying that Andre is doing nothing wrong here; it's a matter of setting the bar for making generic functions part of DPDK's public API.

In this particular case, I don't have a strong opinion on how public the new function is.
Putting it in some generic private header is also perfectly acceptable for me.
Just don't put it directly in the driver; that would lead to copy-paste into other drivers.

> 
> >
> > The only issue I have with these (incl. the bit operations) are that
> they are in the EAL library, although they have absolutely nothing to
> do with hardware or O/S abstraction, so they really should be in a
> "utils" library.
> > But that's another story, so let's not burden Andre with that.
> 
> Orthogonal to the question.

Partly, yes.
But if we had a generic "utils" library, there would be less resistance to adding the new function there than there is to adding it to the EAL API.