From: Bill Fischofer [mailto:bill.fischo...@linaro.org] Sent: Thursday, July 07, 2016 3:01 PM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia-bell-labs.com> Cc: lng-odp@lists.linaro.org Subject: Re: [lng-odp] [PATCH] linux-generic: byteorder: avoid bitfield order doxygen omissions
On Thu, Jul 7, 2016 at 6:57 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia-bell-labs.com> wrote: From: Bill Fischofer [mailto:bill.fischo...@linaro.org] Sent: Thursday, July 07, 2016 2:42 PM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia-bell-labs.com> Cc: lng-odp@lists.linaro.org Subject: Re: [lng-odp] [PATCH] linux-generic: byteorder: avoid bitfield order doxygen omissions On Thu, Jul 7, 2016 at 2:46 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia-bell-labs.com> wrote: I'm OK with the change, but didn't do it as part of my patch set since it's an API change. I think the change is not a big deal and should be done to align the usage of byte/bitfield defines. The patch should be tagged with api: prefix and go through api-next. Also, API spec could be more explicit about the byte/bitfield defines and how application should use/test those. I think bitfield defines should work the same way as the current byte order defines. #if (ODP_BYTE_ORDER == ODP_BIG_ENDIAN) // big endian byte order #else // little endian byte order #endif -Petri Not making an API change was the reason I did this change this way. This is now an implementation change only. The change that would involve an API change would have been to define an ODP_BITFIELD_ORDER variable to mirror the ODP_BYTE_ORDER variable and set that to either ODP_LITTLE_ENDIAN_BITFIELD or ODP_BIG_ENDIAN_BITFIELD as needed. > --- a/platform/linux-generic/include/protocols/tcp.h > +++ b/platform/linux-generic/include/protocols/tcp.h > @@ -34,7 +34,7 @@ typedef struct ODP_PACKED { > odp_u32be_t ack_no; /**< Acknowledgment number */ > union { > odp_u16be_t doffset_flags; > -#if defined(ODP_BIG_ENDIAN_BITFIELD) > +#if ODP_BIG_ENDIAN_BITFIELD The change is non-backward, API change for application code. Tcp helper is also application code. Today, application may do this half of the time ... #if defined(ODP_BIG_ENDIAN_BITFIELD) // foo #else // bar #endif ... and this the other half ... #if defined(ODP_LITTLE_ENDIAN_BITFIELD) // foo #else // bar #endif After the change, application would break since both ODP_LITTLE_ENDIAN_BITFIELD and ODP_BIG_ENDIAN_BITFIELD are defined always. Yes, which is why the patch includes an update to both copies of tcp.h, which are currently the only files that reference those #defines. The issue is that the recent change seems to have broken doxygen, so the question is what's the best way to fix that? << Reply to HTML mail>> You must consider helpers as part of application code. There may be other application code out there which does exactly the same thing (the #if defined example above). This change would break *any* application code that uses the defines. -Petri
