Re: [PATCH rdma-core] ccan: Add likely implementation

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On Wed, Nov 16, 2016 at 01:13:43PM -0700, Jason Gunthorpe wrote:
> On Wed, Nov 16, 2016 at 07:40:11PM +0200, Leon Romanovsky wrote:
>
> > index b5de515..153426f 100644
> > +++ b/ccan/CMakeLists.txt
> > @@ -6,11 +6,13 @@ publish_internal_headers(ccan
> >    minmax.h
> >    str.h
> >    str_debug.h
> > +  likely.h
> >    )
> >
> >  set(C_FILES
> >    list.c
> >    str.c
> > +  likely.c
> >    )
>
> Keep these lists sorted please

Sure

>
> > +++ b/ccan/likely.c
> > @@ -0,0 +1,136 @@
> > +/* CC0 (Public domain) - see LICENSE file for details. */
> > +#ifdef CCAN_LIKELY_DEBUG
> > +#include <ccan/likely/likely.h>
> > +#include <ccan/hash/hash.h>
> > +#include <ccan/htable/htable_type.h>
>
> Hmm, this isn't going to compile if the debug is set, maybe drop the
> .c - but this seems really interesting to see if likely is being used
> sensibly....
>
> > +#ifndef CCAN_LIKELY_DEBUG
> > +#if HAVE_BUILTIN_EXPECT
>
> You need to add '#define HAVE_BUILTIN_EXPECT 1' to
> buildlib/config.h.in - or this doesn't work at all.

This is exactly what I wanted to discuss over ML.

From one side, I wanted to ensure that ccan files are similar to
official ones, so upgrade to new versions will be seamless.

From another side, I don't see the real usage of likely/unlikely debug
facilities.

So my approach was to add these files as is, but don't connect debug
functionality.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]