Hi,
Here we go for a late reply...
On Wed, 7 Sep 2005, Daniel Rall wrote:
> On Thu, 18 Aug 2005, Peter N. Lundblad wrote:
>
> > > void
> > > -svn_intl_set_locale_prefs (void *context, char **locale_prefs);
> > > +svn_intl_set_locale_prefs (char **locale_prefs, apr_pool_t *pool);
> > >
> > locale_prefs should be const.
>
> Should it also be const as an apr_array_header_t **?
>
Yep.
>
> > > + /* ### Do we need to explicitly supply a destructor for the
> > > + ### thread-local storage? This module's private pool and our
> > > + ### server's threads don't necessarily have the same
> > > + ### lifetime. */
> >
> > What do you need to destroy?
> >
> > > + void *destructor_func = NULL;
> > > + apr_threadkey_private_create (&locale_prefs_threadkey,
> > > + destructor_func, private_pool);
> >
> > Why not just NULL instead of that destructor_func?
>
> I don't fully grasp how this API works (e.g. is the destructor even needed?),
> so the dtor is there as a place-holder to remind me to come back and take a
> look at this. Advice welcome.
>
The destructor is called with the key's value when a thread terminates, so
if you need a per-thread pool for this or something, you may need to use
the destructor. *But*, the destructor is not called when the threadkey is
destroyed.
> > > @@ -97,12 +117,16 @@
> > > of its environment. */
> > > if (setlocale(LC_ALL, "") == NULL)
> > > {
> > > + /* setlocale() failed -- inspect the env vars it checks, and
> > > + report an error when we encounter one of them with a
> > > + value set. */
> > > const char *env_vars[] = { "LC_ALL", "LC_CTYPE", "LANG", NULL };
> > > const char **env_var = &env_vars[0], *env_val = NULL;
> > > while (*env_var)
> > > {
> > > env_val = getenv(*env_var);
> > > if (env_val && env_val[0])
> > > + /* The environment variable is set to some value. */
> >
> > why comment the obvious? :-)
>
> I found this block somewhat hard to understand. Looking at the comment
> alone, it's rather useless, but it's meant to go hand-in-hand with the
> comment added at the top of the block.
>
OK, matter of taste.
> > > /* The bundle could be missing the "translation", or we could be
> > > missing a bundle for the locale. */
> > > - if (apr_strnatcmp(locale, SVN_CLIENT_MESSAGE_LOCALE) != 0)
> > > + if (apr_strnatcmp (locale, SVN_CLIENT_MESSAGE_LOCALE) != 0)
> >
> > Please tell an uninformed why you use apr_strnatcmp instead of a plain
> > strcmp. (I'm not sure what apr_strnatcmp does).
>
> Cut-n-pate from config-test.c. There's no reason to it use apr_strnatcmp()
> instead of strcmp(). I've changed it in both places.
>
> ...
> > > void
> > > -svn_intl_set_locale_prefs (void *context, char **locale_prefs)
> > > +svn_intl_set_locale_prefs (char **locale_prefs, apr_pool_t *pool)
> > > {
> > > - /* ### TODO: Save locale information to thread-local storage. */
> > > -
> > > + if (locale_prefs_threadkey != NULL)
> > > + {
> > > + /* ### TODO: Cleanup any previously set locale preferences. */
> > > + apr_threadkey_private_set (locale_prefs, locale_prefs_threadkey);
> > > + }
> > >
> > You should copy the prefs to some local storage.
>
> Done.
>
> > > /* ### LATER: Should we save the locale information to the context
> > > ### instead? For instance, if context was an apr_pool_t, we
> > > @@ -299,25 +326,33 @@
> > > const char *
> > > svn_intl_dgettext (const char *domain, const char *msgid)
> > > {
> > > - const char *locale;
> > > - /* See http://apr.apache.org/docs/apr/group__apr__thread__proc.html */
> > > - apr_threadkey_t *key;
> > > - /* ### use sub-pool? */
> > > - apr_threadkey_private_create (&key, NULL /* ### */, private_pool);
> > > - apr_threadkey_private_get (&locale, key);
> > > - apr_threadkey_private_delete (key);
> > > + const char *text;
> > >
> > > - if (locale == NULL)
> > > + char **locale_prefs = svn_intl_get_locale_prefs (private_pool);
> >
> > Since svn_intl_get_locale_prefs might allocate in pool, this might leak.
>
> I punted. I need a pool per the function decl, but there's none to be had.
> svn_intl_get_locale_prefs() will (currently) allocate from this pool if it
> falls back to the thread's locale. If I changed the semantics such that
> the svn_intl_get_locale_prefs() did not consider the current thread's locale
> to be an actual preference, and relied on the consideration of the current
> thread's locale in svn_intl_dgettext(), the pool could be removed from the
> function decl. Alternately (and perhaps the actual solution),
> svn_intl_dgettext() and svn_intl_dlgettext() could be made to take pool
> arguments. However, this would require that the _() macros accepted pool
> arguments (which I'm not quite ready for). What do you think?
>
If we change from TSD to storing the locale in a pool, we'll need a pool
in the _() macro as well. It's some work, but it also has some benefits.
Some time ago, we discussed this on IRC. Then we had a good argument for
using pools instead of TSD. But I can't remember it now. In any case, you
can't use private_pool here. And we need to be careful not to make a _()
call too heavy.
Greetings,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Sep 17 16:28:10 2005