Julian Foad writes:
> Paul Burba wrote:
> >
> > Here is my first stab at this.
> >
> > While it isn't strictly necessary, I changed all callers to
> > svn_utf_cstring_*_utf8_ex() which passed a non-NULL convset_key to now
> > pass NULL. This makes it clearer to someone working on the calling code
> > that this argument is no longer relevant.
>
> Good.
This makes me question my suggestion not to rev the API. Obviously,
you should change the arguments to null everywhere, and then it
doesn't take much to rev the APIs. Well...
> > Index: subversion/libsvn_subr/utf.c
> > ===================================================================
> > --- subversion/libsvn_subr/utf.c (revision 19012)
> > +++ subversion/libsvn_subr/utf.c (working copy)
> > @@ -729,6 +729,22 @@
> > xlate_handle_node_t *node;
> > svn_error_t *err;
> >
> > + /* In the cases of SVN_APR_LOCALE_CHARSET and SVN_APR_DEFAULT_CHARSET
> > + * frompage is really an int, not a valid string. So generate a unique
> > + * key accordingly. The exception is OS400 where code pages are always
> > + * ints. */
> > +#ifndef AS400
> > + if (frompage == SVN_APR_LOCALE_CHARSET
> > + || frompage == SVN_APR_DEFAULT_CHARSET)
> > +#endif
> > + convset_key = apr_psprintf(pool, "svn-utf-%dtou-xlate-handle",
> > + (int)frompage);
> > +#ifndef AS400
> > + else
> > + convset_key = apr_psprintf(pool, "svn-utf-%stou-xlate-handle",
> > + frompage);
> > +#endif
> > +
> > SVN_ERR(get_xlate_handle_node(&node, SVN_APR_UTF8_CHARSET, frompage,
> > convset_key, pool));
>
> That looks like it will work, but I was hoping we could do something more
> efficient, along the lines of:
>
> * Use the object "struct {const char *frompage, topage; }" as the hash key, not
> caring whether the two pointers are real pointers or type-cast integers.
NOthing guarantees that two string constants that are equal share the
same address, even if the compiler *might* optimize that way. But you
could probably save some cycles by using apr_pstrcat instead of
sprintf and that's as easy to read and maintain.
> That would be done in a single place, in get_xlate_handle_node().
>
> Presently, the code uses either a dedicated, global-ish hash
> "xlate_handle_hash" if our UTF-8 sub-system has been initialised
> (svn_utf_initialize()), otherwise the "user data" hash of whatever pool the
> caller supplies.
>
> When using the pool user data as the hash, there are two additional
> requirements on the key: it must be a null-terminated cstring (because the user
> data access API requires this), and it must be a unique string within the
> program - hence the "svn-utf-" prefix and "-xlate-handle" suffix.
>
> In order to use a bare pointer pair as a hash key, we must always use our own
> hash: we can't go looking up this key directly in the pool userdata hash.
> Therefore we must do one of:
>
> * Require that svn_utf_initialize() be used, which means we always use the
> dedicated "xlate_handle_hash". We can remove the other code path except in the
> backwards-compatibility API. I think this requires "revving" the API, but I
> don't see that as a problem (indeed, it would be good because we could
> eliminate the redundant arguments).
>
> or
>
> * Store a translation handle hash (which uses our pointer pair as its key)
> within the pool userdata (which can then use a single, constant string as its key).
>
I think both of these are too messy for little gain.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Mar 25 21:53:44 2006