On Thu, Jul 7, 2016 at 8:38 AM, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote:
> > > From: Bill Fischofer [mailto:[email protected]] > Sent: Thursday, July 07, 2016 3:01 PM > To: Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]> > Cc: [email protected] > 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) < > [email protected]> wrote: > > > From: Bill Fischofer [mailto:[email protected]] > Sent: Thursday, July 07, 2016 2:42 PM > To: Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]> > Cc: [email protected] > 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) < > [email protected]> 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. > So short of reverting your latest header restructure patch, how would you suggest fixing this? > > -Petri > > > > > > > >
