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