stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Date Thu, 03 Apr 2008 19:02:12 GMT
Eric Lemings wrote:
[...]
> Please peer review the following patch at your earliest convenience.
> 
> -----
> $ svn diff
> Index: src/setlocale.h
> ===================================================================
> --- src/setlocale.h     (revision 644426)
> +++ src/setlocale.h     (working copy)
> @@ -58,6 +58,9 @@
>          return _C_name;
>      }
> 
> +    static const char* __rw_curlocale (int);
> +    static bool __rw_newlocale (const char*, int);
> +
>  private:
> 
>      __rw_setlocale (const __rw_setlocale&);   // not defined
> Index: src/setlocale.cpp
> ===================================================================
> --- src/setlocale.cpp   (revision 644426)
> +++ src/setlocale.cpp   (working copy)
> @@ -80,11 +80,8 @@
>  __rw_setlocale::__rw_setlocale (const char *locname, int cat, int
> nothrow)
>      : _C_name (0), _C_guard (1), _C_cat (cat)
>  {
> -    // acquire global per-process lock
> -    __rw_setlocale_mutex._C_acquire ();
> -
>      // retrieve previous locale name and check if it is already set
> -    const char* const curname = ::setlocale (cat, 0);
> +    const char* const curname = __rw_curlocale (cat);
> 
>      if (curname) {
> 
> @@ -101,14 +98,13 @@
> 
>          ::memcpy (_C_name, curname, size);

This doesn't seem safe: some other thread might have called
setlocale by now and invalidated our curname. The ctor needs
to hold the lock until it's finished copying the name of the
locale returned by setlocale().

IMO, the least invasive change to __rw_setlocale is to expose
the __rw_setlocale_mutex object (currently defined static in
setlocale.cpp) so that it can be used with the _RWSTD_MT_GUARD()
macro. That way we won't have to change any existing
__rw_setlocale code. The fix to locale::global() will then
be to add

     _RWSTD_MT_GUARD (__rw_setlocale::_C_mutex);

at the top of the function.

Martin

> 
> -        if (::setlocale (cat, locname)) {
> +        if (__rw_newlocale (locname, cat)) {
> 
>              // a NULL name is only used when we want to use the object
>              // as a retriever for the name of the current locale;
>              // no need for a lock then
>              if (!locname) {
>                  _C_guard = 0;
> -                __rw_setlocale_mutex._C_release ();
>              }
> 
>              return;
> @@ -122,7 +118,6 @@
> 
>      // bad locales OR bad locale names OR bad locale categories
>      _C_guard = 0;
> -    __rw_setlocale_mutex._C_release ();
> 
>      // failure in setting the locale
>      _RWSTD_REQUIRES (nothrow, (_RWSTD_ERROR_LOCALE_BAD_NAME,
> @@ -131,6 +126,25 @@
>  }
> 
> 
> +/*static*/ const char*
> +__rw_setlocale::__rw_curlocale (int cat)
> +{
> +    __rw_setlocale_mutex._C_acquire ();
> +    const char* const curname = ::setlocale (cat, 0);
> +    __rw_setlocale_mutex._C_release ();
> +    return curname;
> +}
> +
> +/*static*/ bool
> +__rw_setlocale::__rw_newlocale (const char* locname, int cat)
> +{
> +    __rw_setlocale_mutex._C_acquire ();
> +    bool changed = (0 != ::setlocale (cat, locname));
> +    __rw_setlocale_mutex._C_release ();
> +    return changed;
> +}
> +
> +
>  // dtor restores the C library locale that
>  // was in effect when the object was constructed
>  __rw_setlocale::~__rw_setlocale ()
> Index: src/locale_global.cpp
> ===================================================================
> --- src/locale_global.cpp       (revision 644426)
> +++ src/locale_global.cpp       (working copy)
> @@ -54,8 +54,7 @@
> 
>          // assumes all locale names (i.e., including those of combined
>          // locales) are in a format understandable by setlocale()
> -        const _RW::__rw_setlocale clocale (rhs.name().c_str(),
> _RWSTD_LC_ALL);
> -        _RWSTD_UNUSED(clocale);
> +        _RW::__rw_setlocale::__rw_newlocale (rhs.name().c_str(),
> _RWSTD_LC_ALL);
> 
>          // FIXME:
>          // handle unnamed locales (i.e., locales containing
> non-standard
> -----
> 
> Thanks,
> Brad.


Mime
View raw message