On Thu, 18 Aug 2005, Peter N. Lundblad wrote:
> On Tue, 16 Aug 2005 dlr@tigris.org wrote:
>
> > Author: dlr
> > Date: Tue Aug 16 16:45:57 2005
> > New Revision: 15766
> >
> Some comments and questions.
>
> > Modified: branches/server-l10n/subversion/include/svn_error_codes.h
> > Url: http://svn.collab.net/viewcvs/svn/branches/server-l10n/subversion/include/svn_error_codes.h?rev=15766&p1=branches/server-l10n/subversion/include/svn_error_codes.h&p2=branches/server-l10n/subversion/include/svn_error_codes.h&r1=15765&r2=15766
> > ==============================================================================
> > --- branches/server-l10n/subversion/include/svn_error_codes.h (original)
> > +++ branches/server-l10n/subversion/include/svn_error_codes.h Tue Aug 16 16:45:57 2005
...
> > @@ -792,6 +794,11 @@
> > SVN_ERRDEF (SVN_ERR_AUTHZ_UNWRITABLE,
> > SVN_ERR_AUTHZ_CATEGORY_START + 4,
> > "Item is not writable")
> > +
> > + /* @since New in 1.3 */
>
> Should be double star.
Ah! I blindly followed the style used throughout this header. All Doxygen
in svn_error_codes.h is missing the double star on the "start" token. I've
fixed my addition this branch, and the remainder on the trunk.
...
> > Modified: branches/server-l10n/subversion/include/svn_intl.h
> > Url: http://svn.collab.net/viewcvs/svn/branches/server-l10n/subversion/include/svn_intl.h?rev=15766&p1=branches/server-l10n/subversion/include/svn_intl.h&p2=branches/server-l10n/subversion/include/svn_intl.h&r1=15765&r2=15766
> > ==============================================================================
> > --- branches/server-l10n/subversion/include/svn_intl.h (original)
> > +++ branches/server-l10n/subversion/include/svn_intl.h Tue Aug 16 16:45:57 2005
...
> > -/* Gets the locale preferences for @a context. If @a context is @c
> > - NULL, gets the system locale for the current process. Returns a
> > - list of locales ordered by preference. Never returns @c NULL, but
> > - may return an empty array (e.g. consisting only of a single @c NULL
> > - value). */
> > -char **
> > -svn_intl_get_locale_prefs (void *context, apr_pool_t *pool);
> > +/* Gets the locale preferences for the current context, falling back
> > + to the system locale for the current process if no user preferences
> > + have been set. Returns a list of locales ordered by preference.
> > + Never returns @c NULL, but may return an empty array
> > + (e.g. consisting only of a single @c NULL value). */
> > +const char **
>
> I think "Gets" should be "Return" to be consistent with our docstring
> style. Should should mention the lifetime of the returned data (IMO it
> should be alloced in @a pool).
Done.
> And say that the return value is a NULL-terminated list...
Ben reminded me that use of an apr_array_header_t would be more consistent
with the rest of SVN's APIs, so I'm changing this code to use that instead.
(I had this debate with myself when originally introducing the API, but the
dastardly Voices prevailed.)
> > +svn_intl_get_locale_prefs (apr_pool_t *pool);
> >
> > -/* Sets the locale preferences for @a context. @a locale_prefs are
> > - inspected in order for a matching resource bundle. Not invoking
> > - this API, or invoking it with a @c NULL locale, or finding no match
> > - against the preferences will result in the global locale being used
> > - instead. */
> > +/* Sets the locale preferences for the current user. @a locale_prefs
>
> s/Sets/Set/
>
> What is the "user" in this context? Yeah, we aren't sure yet if it is
> thread or pool, just want to point out that "user" isn't very clear.
I tweaked the doc string to be more consistent with the
svn_intl_get_locale_prefs() function. I'm not sure this sufficiently
clarified things (but at least they're consistently vague now ;-). I may
need to revisit this after switching from thread-local storage to pool-based
storage.
> > + are inspected in order for a matching resource bundle. Not
> > + invoking this API, or invoking it with a @c NULL locale, or finding
> > + no match against the preferences will result in the global locale
> > + being used instead. */
>
> Not sure what the global locale is. Is it the locale of the
> process (thread on Windows) or the "C" locale?
It's the return value of setlocale(), so the locale of the process.
I've adjusted the doc string accordingly.
> > 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 **?
...
> > Modified: branches/server-l10n/subversion/libsvn_subr/intl.c
> > Url: http://svn.collab.net/viewcvs/svn/branches/server-l10n/subversion/libsvn_subr/intl.c?rev=15766&p1=branches/server-l10n/subversion/libsvn_subr/intl.c&p2=branches/server-l10n/subversion/libsvn_subr/intl.c&r1=15765&r2=15766
> > ==============================================================================
> > --- branches/server-l10n/subversion/libsvn_subr/intl.c (original)
> > +++ branches/server-l10n/subversion/libsvn_subr/intl.c Tue Aug 16 16:45:57 2005
...
> > @@ -40,9 +40,13 @@
> >
> > static apr_hash_t *cache;
> > static apr_pool_t *private_pool = NULL;
> > -#if APR_HAS_THREADS
> > +
> > +/* A mutex around writes to CACHE. */
> > static apr_thread_mutex_t *cache_lock;
> > -#endif
> > +
>
> AFAICT, apr_thread_mutex_t isn't available if APR_HAS_THREADS is not
> defined.
Yeah, you're right. I removed this because svn_intl_terminate() referenced
cache_lock outside of a #if APR_HAS_THREADS. I've reverted this change, and
applied the correct fix to svn_intl_terminate().
> > + /* ### 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.
> > @@ -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.
> > /* 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 (locale_prefs != NULL)
> > {
> > - /* ### A shortcut to avoid dealing with locale-related env vars,
> > - ### GetThreadLocale(), etc. Ideally, we'd used only one
> > - ### gettext-like implementation which suites our purposes. */
> > - return dgettext(domain, msgid);
> > + /* Attempt to find a localization matching the specified locale
> > + preferences. */
>
> s/localization/translation/
Yeah, I noticed that the term "translation" was used in a lot of places in
SVN. A translation is a specialization of a localization, the latter being
the term I've always used to refer to any locale-specific data (be it
textual, image, etc.). This is indeed specific to text, so I've changed it
as you suggest.
> > + char *locale;
> > + for (locale = locale_prefs[0]; locale_prefs != NULL; locale_prefs++)
> > + {
> > + text = svn_intl_dlgettext (domain, locale, msgid);
> > + if (text != NULL && apr_strnatcmp (msgid, text) != 0)
>
> So, svn_intl_dlgettext can both return NULL and the same string? And what
> says that the same string is not a legitimate translation (for example, in
> a .po file for Brittish English file many "translations" will be the
> same). (The last time I looked into GCC sources, there was a .po for
> Brittish English.)
>
> gettext's way of saying "no translation found" is to return its argument,
> so a simple pointer comparison will do. I think we should do the same.
I agree, and have added that to the doc string for svn_intl_dlgettext().
> that would also avoid this mysterious apr_strnatcmp...
Welcome to my haunted house. :-)
> > + /* Localization found, stop looking. */
>
> s/Localization/Translation/
Done.
Peter, thanks a ton for the review! r16032 on the server-l10n branch contains
the majority of the adjustments.
- Dan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 7 17:48:35 2005