Date: Thu, 12 Apr 2012 13:57:29 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 4/4] Explicitly type void* pointers
Message-ID: <20120412175729.GH13549@mit.edu>
References: <1333966665-10469-1-git-send-email-Vladimir.Marek@oracle.com>
	<1333966665-10469-5-git-send-email-Vladimir.Marek@oracle.com>
	<CAB+hUn_QyMAfC1-Dr23T63ObX=cwuBOzcyCi=61w0MtGRMyt2w@mail.gmail.com>
	<20120409181543.GC10554@pub.czech.sun.com>
	<87mx6ka8y1.fsf@nikula.org>
	<87vcl6nfda.fsf@awakening.csail.mit.edu>
	<87aa2hxtrv.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87aa2hxtrv.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

Quoth Jani Nikula on Apr 12 at  8:02 am:
> On Wed, 11 Apr 2012 17:11:13 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > On Mon, 09 Apr 2012, Jani Nikula <jani@nikula.org> wrote:
> > > Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:
> > > I'm throwing in a third alternative below. Does it work for you? I think
> > > it's both prettier and uglier than the above at the same time! ;)
> > >
> > > A middle ground would be to change the callers to use
> > > "notmuch_talloc_steal", and just #define notmuch_talloc_steal
> > > talloc_steal if __GNUC__ >= 3.
> > >
> > > One could argue upstream talloc should have this, but OTOH it's a C
> > > library.
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> > > index ea836f7..83b46e8 100644
> > > --- a/lib/notmuch-private.h
> > > +++ b/lib/notmuch-private.h
> > > @@ -499,4 +499,22 @@ _notmuch_filenames_create (const void *ctx,
> > >  
> > >  NOTMUCH_END_DECLS
> > >  
> > > +#ifdef __cplusplus
> > > +/* Implicit typecast from 'void *' to 'T *' is okay in C, but not in
> > > + * C++. In talloc_steal, an explicit cast is provided for type safety
> > > + * in some GCC versions. Otherwise, a cast is required. Provide a
> > > + * template function for this to maintain type safety, and redefine
> > > + * talloc_steal to use it.
> > > + */
> > > +#if !(__GNUC__ >= 3)
> > > +template <class T>
> > > +T *notmuch_talloc_steal(const void *new_ctx, const T *ptr)
> > > +{
> > > +    return static_cast<T*>(talloc_steal(new_ctx, ptr));
> > > +}
> > > +#undef talloc_steal
> > > +#define talloc_steal notmuch_talloc_steal
> > > +#endif
> > > +#endif
> > > +
> > >  #endif
> > 
> > This looks good to me.  I was originally concerned that this depended on
> > talloc_steal being a macro, but I realized that's not actually the case.
> > Care to roll a real patch?
> 
> Sure.
> 
> One question: the template must be outside NOTMUCH_{BEGIN,END}_DECLS
> (which are just macros for extern "C" block) but should it be within the
> #pragma GCC visibility push(hidden) and pop directives? I'm not familiar
> with that.

Hmm.  I'm not sure, but it can't hurt to have it inside the visibility
directives.  My guess would be that it will affect the visibility of
the symbols produced when the template is instantiated, which we don't
want to be visible outside the library.  (As long as that doesn't mess
up the linker's handling of weak symbols.)

> Thanks for the review.
> 
> 
> BR,
> Jani.