> From: David Marchand [mailto:david.march...@redhat.com] > Sent: Wednesday, 4 June 2025 12.41 > > On Wed, Jun 4, 2025 at 12:29 PM Morten Brørup > <m...@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.
