Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[Thomas Monjalon <thomas@monjalon.net>]

01/07/2019 22:41, Bruce Richardson:
> On Mon, Jul 01, 2019 at 09:30:02PM +0200, Thomas Monjalon wrote:
> > 29/05/2019 17:41, Bruce Richardson:
> > > Use the flag checking functions and a couple of empty stubs to remove the
> > > ifdefs from the middle of the C code, and replace them with more readable
> > > regular if statements. Other ifdefs at the top of the file are kept, but
> > > are not mixed with C code, so there is a clean separation.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
> > >  1 file changed, 25 insertions(+), 13 deletions(-)
> > 
> > The result is more lines of code :)
> > 
> > > --- a/lib/librte_net/rte_net_crc.c
> > > +++ b/lib/librte_net/rte_net_crc.c
> > > @@ -18,8 +18,17 @@
> > >  
> > >  #ifdef X86_64_SSE42_PCLMULQDQ
> > >  #include <net_crc_sse.h>
> > > -#elif defined ARM64_NEON_PMULL
> > > +#else
> > > +/* define stubs for the SSE functions to avoid compiler errors */
> > > +#define handlers_sse42 handlers_scalar
> > > +#define rte_net_crc_sse42_init() do { } while(0)
> > > +#endif
> > > +
> > > +#ifdef ARM64_NEON_PMULL
> > >  #include <net_crc_neon.h>
> > > +#else
> > > +#define handlers_neon handlers_scalar
> > > +#define rte_net_crc_neon_init() do { } while(0)
> > >  #endif
> > 
> > Looking at the need for stubs, I don't see the benefit.
> >
> Yes, one needs stubs, but those are placed in a single place, and the main
> C-code itself is free of ifdefs running through it. I'd view this as a good
> thing in limiting the scope of any ifdef-ery, since it annoys me looking at
> #ifdefs in the middle of functions (since it messes up the indentation flow
> of the code if nothing else!).
> 
> If you don't see this as a big benefit, then there is not a lot else to
> commend this set for you, sadly. It just seemed a nice improvement to me -
> irrespective of net lines of code.

Any other opinion?