Re: svn commit: r12144 - trunk/subversion/bindings/swig/perl/libsvn_swig_perl

On Sat, Dec 04, 2004 at 09:13:59AM -0600, [email protected] wrote:
> Author: clkao
> Date: Sat Dec  4 09:13:55 2004
> New Revision: 12144
> 
> Modified:
>    trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c
> Log:
> r11300@ab:  clkao | 2004-11-17T15:48:54.151638Z
> * Pass pool to methods in Perl for close_baton.
> * Use a hash to cache SWIG_TypeQuery results.
> 
>   * libsvn_swig_perl/swigutil_pl.c:
>     (close_baton): Pass pool to methods in Perl.
>       Use svn_swig_pl_callback_thunk.
> 
>     (_swig_perl_type_query): New.
>     (_SWIG_TYPE): New.
>  
>     Change callers of SWIG_TypeQuery to use _swig_perl_type_query.
> 
> 
> 
> Modified: trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c?view=diff&rev=12144&p1=trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c&r1=12143&p2=trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c&r2=12144
> ==============================================================================
> --- trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c	(original)
> +++ trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c	Sat Dec  4 09:13:55 2004
> @@ -31,6 +31,32 @@
>  
>  #include "swigutil_pl.h"
>  
> +/* cache SWIG_TypeQuery results in a perl hash */
> +static HV *type_cache = NULL;
> +
> +#define _SWIG_TYPE(name) _swig_perl_type_query(name, sizeof (name)-1)

The above macro is bad.  It references the paramter name twice.  You
shouldn't do that.  In this particular case it doesn't break anything
from the existing usages.  But it might in the future.

Additionally _swig_perl_type_query wants the strlen not the size of the
pointer.  Probably should just use:

#define _SWIG_TYPE(name) _swig_perl_type_query(name, 0);

since....

> +#define POOLINFO         _SWIG_TYPE("apr_pool_t *")
> +
> +static swig_type_info *_swig_perl_type_query (const char *typename, U32 klen)
> +{
> +    SV **type_info;
> +    swig_type_info *tinfo;
> +
> +    if (!type_cache)
> +        type_cache = newHV ();
> +
> +    if (klen == 0)
> +        klen = strlen (typename);

You've already allowed 0 to be passed to the function to calculate the
strlen for you.  No since in putting it in the macro then.  Especially
since you have to jump through hoops to make that a safe macro.

> +    if ((type_info = hv_fetch(type_cache, typename, klen, 0)))
> +        return (swig_type_info *) (SvIV (*type_info));
> +
> +    tinfo = SWIG_TypeQuery(typename);
> +    hv_store(type_cache, typename, klen, newSViv ((IV)tinfo), 0);
> +
> +    return tinfo;
> +}

[snip] rest of it looks fine.

-- 
Ben Reser <[email protected]>
http://ben.reser.org

"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken