[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r15766 - in branches/server-l10n/subversion: include libsvn_ra_dav libsvn_subr tests/libsvn_subr

From: Peter N. Lundblad <peter_at_famlundblad.se>
Date: 2005-08-18 09:36:32 CEST

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
> @@ -138,6 +138,8 @@
> + (19 * SVN_ERR_CATEGORY_SIZE))
> #define SVN_ERR_AUTHZ_CATEGORY_START (APR_OS_START_USERERR \
> + (20 * SVN_ERR_CATEGORY_SIZE))
> +#define SVN_ERR_NLS_CATEGORY_START (APR_OS_START_USERERR \
> + + (21 * SVN_ERR_CATEGORY_SIZE))
>
> #endif /* DOXYGEN_SHOULD_SKIP_THIS */
>
> @@ -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.

> + SVN_ERRDEF (SVN_ERR_NLS_UNRECOGNIZED_LOCALE,
> + SVN_ERR_NLS_CATEGORY_START + 0,
> + "Locale is not recognized")
>
> /* svndiff errors */
>
>
> 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
> @@ -39,21 +39,21 @@
> svn_error_t *
> svn_intl_initialize (apr_pool_t *parent_pool);
>
> -/* 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). And say that the return value is a
NULL-terminated list...

> +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.

> + 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?

> 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.

> /* Retrieve the text identified by @a msgid for the text bundle
> corresponding to @a domain and @a locale. */
>
> 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
> @@ -19,7 +19,7 @@
> #include <apr_mmap.h>
> #include <apr_file_io.h>
> #include <apr_strings.h>
> -#include <apr_thread_proc.h>
> +#include <apr_thread_proc.h> /* for thread-local APIs */
> #include <assert.h>
> #include <apr_hash.h>
> #include <ctype.h>
> @@ -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.

> + /* ### 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?

> @@ -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? :-)

> /* 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).

> {
> - prefs = apr_pcalloc(pool, 2 * sizeof(char *));
> + prefs = apr_pcalloc (pool, 2 * sizeof(char *));
> prefs[0] = locale;
> }
> }
> return prefs;
> }
>
> +
> 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.

> /* ### 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.

> + 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/

> + 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.
that would also avoid this mysterious apr_strnatcmp...

> + /* Localization found, stop looking. */

s/Localization/Translation/

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 18 09:37:45 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.