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

[Thomas Monjalon <thomas@monjalon.net>]

04/06/2025 13:04, Morten Brørup:
> > 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.

We can move it as a public API later if there is a real need.
I don't think it is good to rush on adding new API in general.


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

EAL is also good to abstract compiler differences.
I agree such basic stuff should be in EAL
if we would decide to add it.

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

It would be the same.
An API is supposed to be maintained forever
and give guidance on what to use.

As a conclusion, I agree with David, it is safe to keep it private
for the unused base function for now.