Re: [so_7-2] DR 13631 patch

[François Dumont <frs dot dumont at gmail dot com>]

Here is what I have finally commited to libstdcxx_so_7-2 branch.

2012-03-01  François Dumont <fdumont@gcc.gnu.org>

        DR libstdc++/13631
        * config/locale/gnu/messages_member.h, messages_member.cc: Prefer
        dgettext usage to gettext to allow usage of several catalogs at the
        same time. Add an internal cache to map catalog names to 
catalog ids.
        * testsuite/22_locale/messages/13631.cc: New.

I have integrated your remarks Paolo and also:
- Add a destructor to the Catalogs class, an application that correctly 
close its catalogs will make this destructor useless but it is normal to 
have one.
- I change the _MapEntry from pair<catalog, string> to pair<catalog, 
const char*>. This way _M_get do not have a string copy anymore.

Tested under linux x86_64.

François

On 02/29/2012 12:02 PM, Paolo Carlini wrote:
> Hi,
> > Hi
> >
> >     I finally spend some more time to enhance this patch.
> >
> >     I used a sorted array to store the mapping, doing so I do not 
> > need to export the _Rb_Tree instantiation anymore. I also simplified 
> > the test, to reproduce the 13631 we don't need an other valid 
> > catalog, any attempt to open a catalog after having open the 
> > 'libstdc++' one show the issue. So we do not need add any 
> > dg-require-XXX macro to validate a catalog presence.
> >
> >     If no one see any problem with this patch I will commit it to 
> > libstdcxx_so_7-2 branch.
> I'm Ok with the patch, besides a few minor stylistic nits below. You 
> didn't post the ChangeLog entry: remember to have the PR number in it.
> > Index: testsuite/22_locale/messages/13631.cc
> > ===================================================================
> > --- testsuite/22_locale/messages/13631.cc	(revision 0)
> > +++ testsuite/22_locale/messages/13631.cc	(revision 0)
> > @@ -0,0 +1,57 @@
> > +// { dg-require-namedlocale "fr_FR" }
> > +
> > +// Copyright (C) 2012 Free Software Foundation
> > +//
> > +// This file is part of the GNU ISO C++ Library.  This library is free
> > +// software; you can redistribute it and/or modify it under the
> > +// terms of the GNU General Public License as published by the
> > +// Free Software Foundation; either version 3, or (at your option)
> > +// any later version.
> > +
> > +// This library is distributed in the hope that it will be useful,
> > +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +// GNU General Public License for more details.
> > +
> > +// You should have received a copy of the GNU General Public License along
> > +// with this library; see the file COPYING3.  If not see
> > +//<http://www.gnu.org/licenses/>.
> > +
> > +#include<locale>
> > +#include<testsuite_hooks.h>
> > +
> > +int main(int argc, char **argv)
> > +{
> > +  bool test __attribute__((unused)) = true;
> > +  // This is defined through CXXFLAGS in scripts/testsuite_flags[.in].
> > +  const char* dir = LOCALEDIR;
> > +
> > +  std::locale l("fr_FR");
> > +
> > +  typedef std::messages<char>  messages;
> > +
> > +  const messages&msgs_facet = std::use_facet<messages>(l);
> > +
> > +  messages::catalog msgs = msgs_facet.open("libstdc++", l, dir);
> > +  VERIFY( msgs>= 0 );
> > +
> > +  const char msgid[] = "please";
> > +  std::string translation1 = msgs_facet.get(msgs, 0, 0, msgid);
> > +
> > +  // Without a real translation this test doesn't mean anything:
> > +  VERIFY( translation1 != msgid );
> > +
> > +  // Opening an other catalog was enough to show the problem, even a fake
> > +  // catalog.
> > +  messages::catalog fake_msgs = msgs_facet.open("fake", l);
> > +
> > +  std::string translation2 = msgs_facet.get(msgs, 0, 0, msgid);
> > +
> > +  // Close catalogs before doing the check to avoid leaks.
> > +  msgs_facet.close(fake_msgs);
> > +  msgs_facet.close(msgs);
> > +
> > +  VERIFY( translation1 == translation2 );
> > +
> > +  return 0;
> > +}
> Normally we have the test proper in a separate funtion called from main.
> > Index: config/locale/gnu/messages_members.cc
> > ===================================================================
> > --- config/locale/gnu/messages_members.cc	(revision 184372)
> > +++ config/locale/gnu/messages_members.cc	(working copy)
> > @@ -1,6 +1,7 @@
> >   // std::messages implementation details, GNU version -*- C++ -*-
> >
> > -// Copyright (C) 2001, 2002, 2005, 2009, 2010 Free Software Foundation, Inc.
> > +// Copyright (C) 2001, 2002, 2005, 2009, 2010, 2012
> > +// Free Software Foundation, Inc.
> >   //
> >   // This file is part of the GNU ISO C++ Library.  This library is free
> >   // software; you can redistribute it and/or modify it under the
> > @@ -31,54 +32,180 @@
> >   #include<locale>
> >   #include<bits/c++locale_internal.h>
> >
> > +#include<algorithm>
> > +#include<utility>
> > +#include<ext/concurrence.h>
> > +
> > +namespace
> > +{
> > +  using namespace std;
> > +
> > +  struct Comp
> > +  {
> > +    typedef messages_base::catalog catalog;
> > +    typedef pair<catalog, string>  _Mapping;
> > +
> > +    bool operator () (catalog __cat, const _Mapping* __pair) const
> > +    { return __cat<  __pair->first; }
> No space after operator
>
> > +
> > +    bool operator () (const _Mapping* __pair, catalog __cat) const
> > +    { return __pair->first<  __cat; }
> Likewise.
>
> > +  };
> > +
> > +  class Catalogs
> > +  {
> > +    typedef messages_base::catalog catalog;
> > +    typedef pair<catalog, string>  _Mapping;
> > +    typedef _Mapping* _MapEntry;
> > +
> > +  public:
> > +    Catalogs() : _M_counter(0), _M_nb_entry(0) { }
> > +
> > +    catalog _M_add(const string&  __s)
> > +    {
> > +      __gnu_cxx::__scoped_lock lock(_M_mutex);
> > +      _MapEntry* __new_map = new _MapEntry[_M_nb_entry + 1];
> > +      _MapEntry __new_entry;
> > +      __try
> > +	{
> > +	  __new_entry = new _Mapping(_M_counter, __s);
> > +	}
> > +      __catch(...)
> > +	{
> > +	  delete[] __new_map;
> > +	  __throw_exception_again;
> > +	}
> > +      copy(_M_map, _M_map + _M_nb_entry, __new_map);
> Normally before a comment we always put a blank line.
>
> In general - I think I noticed something in the unordered_* too - for 
> improved legibility I would recommend packing the code a little less, 
> just add blank lines somewhere, eg, more or less always when an 
> if/for/while/catch curly bracket is closed.
> > +      // The counter is not likely to roll unless catalogs keep on being
> > +      // open/close which is consider as an application mistake for the moment.
> > +      catalog __cat = _M_counter++;
> > +      __new_map[_M_nb_entry] = __new_entry;
> > +      delete[] _M_map;
> > +      _M_map = __new_map;
> > +      ++_M_nb_entry;
> > +      return __cat;
> > +    }
> Thanks,
> Paolo.

Index: config/locale/gnu/messages_members.cc
===================================================================
--- config/locale/gnu/messages_members.cc	(revision 184758)
+++ config/locale/gnu/messages_members.cc	(working copy)
@@ -1,6 +1,7 @@
 // std::messages implementation details, GNU version -*- C++ -*-
 
-// Copyright (C) 2001, 2002, 2005, 2009, 2010 Free Software Foundation, Inc.
+// Copyright (C) 2001, 2002, 2005, 2009, 2010, 2012
+// Free Software Foundation, Inc.
 //
 // This file is part of the GNU ISO C++ Library.  This library is free
 // software; you can redistribute it and/or modify it under the
@@ -31,54 +32,196 @@
 #include <locale>
 #include <bits/c++locale_internal.h>
 
+#include <algorithm>
+#include <utility>
+#include <ext/concurrence.h>
+
+namespace
+{
+  using namespace std;
+
+  typedef messages_base::catalog catalog;
+  typedef pair<catalog, const char*> _MapEntry;
+
+  struct Comp
+  {
+    bool operator()(catalog __cat, const _MapEntry& __entry) const
+    { return __cat < __entry.first; }
+
+    bool operator()(const _MapEntry& __entry, catalog __cat) const
+    { return __entry.first < __cat; }
+  };
+
+  class Catalogs
+  {
+  public:
+    Catalogs() : _M_counter(0), _M_nb_entry(0) { }
+
+    ~Catalogs()
+    {
+      if (_M_nb_entry)
+	{
+	  for (size_t i = 0; i != _M_nb_entry; ++i)
+	    delete[] _M_map[i].second;
+	  delete[] _M_map;
+	}
+    }
+
+    catalog _M_add(const string& __s)
+    {
+      __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+      _MapEntry* __new_map = new _MapEntry[_M_nb_entry + 1];
+      __try
+	{
+	  copy(_M_map, _M_map + _M_nb_entry, __new_map);
+	  char* __s_copy = new char[__s.size() + 1];
+	  __s.copy(__s_copy, __s.size());
+	  __s_copy[__s.size()] = 0;
+	  __new_map[_M_nb_entry] = make_pair(_M_counter, __s_copy);
+	}
+      __catch(...)
+	{
+	  delete[] __new_map;
+	  __throw_exception_again;
+	}
+
+
+      // The counter is not likely to roll unless catalogs keep on being
+      // open/close which is consider as an application mistake for the moment.
+      catalog __cat = _M_counter++;
+      delete[] _M_map;
+      _M_map = __new_map;
+      ++_M_nb_entry;
+
+      return __cat;
+    }
+
+    void _M_erase(catalog __c)
+    {
+      __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+      _MapEntry* __entry =
+	lower_bound(_M_map, _M_map + _M_nb_entry, __c, Comp());
+      if (__entry == _M_map + _M_nb_entry || __entry->first != __c)
+	return;
+      
+      _MapEntry* __new_map =
+	_M_nb_entry > 1 ? new _MapEntry[_M_nb_entry - 1] : 0;
+      copy(__entry + 1, _M_map + _M_nb_entry,
+	   copy(_M_map, __entry, __new_map));
+
+      delete[] __entry->second;
+      delete[] _M_map;
+      _M_map = __new_map;
+      --_M_nb_entry;
+    }
+
+    pair<bool, const char*> _M_get(catalog __c) const
+    {
+      __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+      typedef pair<bool, const char*> _Ret;
+
+      const _MapEntry* __entry =
+	lower_bound(_M_map, _M_map + _M_nb_entry, __c, Comp());
+      if (__entry != _M_map + _M_nb_entry && __entry->first == __c)
+	return _Ret(true, __entry->second);
+      return _Ret(false, 0);
+    }
+
+  private:
+    mutable __gnu_cxx::__mutex _M_mutex;
+    catalog _M_counter;
+    _MapEntry* _M_map;
+    size_t _M_nb_entry;
+  };
+
+  Catalogs&
+  get_catalogs()
+  {
+    static Catalogs __catalogs;
+    return __catalogs;
+  }
+
+  const char*
+  get_glibc_msg(__c_locale __attribute__((unused)) __locale_messages,
+		messages_base::catalog __c,
+		const char* __dfault)
+  {
+    pair<bool, const char*> __ret = get_catalogs()._M_get(__c);
+
+    if (!__ret.first)
+      return __dfault;
+#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 2)
+    std::__c_locale __old = __uselocale(__locale_messages);
+    const char* __msg =
+      const_cast<const char*>(dgettext(__ret.second, __dfault));
+    __uselocale(__old);
+    return __msg;
+#else
+    char* __old = setlocale(LC_ALL, 0);
+    const size_t __len = strlen(__old) + 1;
+    char* __sav = new char[__len];
+    memcpy(__sav, __old, __len);
+    setlocale(LC_ALL, _M_name_messages);
+    const char* __msg = dgettext(__ret.second, __dfault);
+    setlocale(LC_ALL, __sav);
+    delete [] __sav;
+    return __msg;
+#endif
+  }
+}
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
-_GLIBCXX_BEGIN_NAMESPACE_VERSION
+  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // Specializations.
   template<>
+    typename messages<char>::catalog
+    messages<char>::do_open(const basic_string<char>& __s,
+			    const locale&) const
+    { return get_catalogs()._M_add(__s); }
+
+  template<>
+    void
+    messages<char>::do_close(catalog __c) const
+    { get_catalogs()._M_erase(__c); }
+
+  template<>
     string
-    messages<char>::do_get(catalog, int, int, const string& __dfault) const
+    messages<char>::do_get(catalog __c, int, int,
+			   const string& __dfault) const
     {
-#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 2)
-      __c_locale __old = __uselocale(_M_c_locale_messages);
-      const char* __msg = const_cast<const char*>(gettext(__dfault.c_str()));
-      __uselocale(__old);
-      return string(__msg);
-#else
-      char* __old = setlocale(LC_ALL, 0);
-      const size_t __len = strlen(__old) + 1;
-      char* __sav = new char[__len];
-      memcpy(__sav, __old, __len);
-      setlocale(LC_ALL, _M_name_messages);
-      const char* __msg = gettext(__dfault.c_str());
-      setlocale(LC_ALL, __sav);
-      delete [] __sav;
-      return string(__msg);
-#endif
+      if (__c < 0)
+	return __dfault;
+      return string(get_glibc_msg(_M_c_locale_messages,
+				  __c, __dfault.c_str()));
     }
 
 #ifdef _GLIBCXX_USE_WCHAR_T
   template<>
+    typename messages<wchar_t>::catalog
+    messages<wchar_t>::do_open(const basic_string<char>& __s,
+			       const locale&) const
+    { return get_catalogs()._M_add(__s); }
+
+  template<>
+    void
+    messages<wchar_t>::do_close(catalog __c) const
+    { get_catalogs()._M_erase(__c); }
+
+  template<>
     wstring
-    messages<wchar_t>::do_get(catalog, int, int, const wstring& __dfault) const
+    messages<wchar_t>::do_get(catalog __c, int, int,
+			      const wstring& __dfault) const
     {
-# if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 2)
-      __c_locale __old = __uselocale(_M_c_locale_messages);
-      char* __msg = gettext(_M_convert_to_char(__dfault));
-      __uselocale(__old);
-      return _M_convert_from_char(__msg);
-# else
-      char* __old = setlocale(LC_ALL, 0);
-      const size_t __len = strlen(__old) + 1;
-      char* __sav = new char[__len];
-      memcpy(__sav, __old, __len);
-      setlocale(LC_ALL, _M_name_messages);
-      char* __msg = gettext(_M_convert_to_char(__dfault));
-      setlocale(LC_ALL, __sav);
-      delete [] __sav;
-      return _M_convert_from_char(__msg);
-# endif
+      if (__c < 0)
+	return __dfault;
+      return _M_convert_from_char
+	(const_cast<char*>(get_glibc_msg(_M_c_locale_messages, __c,
+					 _M_convert_to_char(__dfault))));
     }
 #endif
 
Index: config/locale/gnu/messages_members.h
===================================================================
--- config/locale/gnu/messages_members.h	(revision 184758)
+++ config/locale/gnu/messages_members.h	(working copy)
@@ -1,6 +1,6 @@
 // std::messages implementation details, GNU version -*- C++ -*-
 
-// Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2009, 2010
+// Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2009, 2010, 2012
 // Free Software Foundation, Inc.
 //
 // This file is part of the GNU ISO C++ Library.  This library is free
@@ -84,22 +84,6 @@
       _S_destroy_c_locale(_M_c_locale_messages); 
     }
 
-  template<typename _CharT>
-    typename messages<_CharT>::catalog 
-    messages<_CharT>::do_open(const basic_string<char>& __s, 
-			      const locale&) const
-    { 
-      // No error checking is done, assume the catalog exists and can
-      // be used.
-      textdomain(__s.c_str());
-      return 0;
-    }
-
-  template<typename _CharT>
-    void    
-    messages<_CharT>::do_close(catalog) const 
-    { }
-
    // messages_byname
    template<typename _CharT>
      messages_byname<_CharT>::messages_byname(const char* __s, size_t __refs)
@@ -127,5 +111,25 @@
 	 }
      }
 
+    template<>
+      typename messages<char>::catalog
+      messages<char>::do_open(const basic_string<char>&,
+			      const locale&) const;
+
+    template<>
+      void
+      messages<char>::do_close(catalog) const;
+
+#ifdef _GLIBCXX_USE_WCHAR_T
+    template<>
+      typename messages<wchar_t>::catalog
+      messages<wchar_t>::do_open(const basic_string<char>&,
+				 const locale&) const;
+
+    template<>
+      void
+      messages<wchar_t>::do_close(catalog) const;
+#endif
+
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
Index: testsuite/22_locale/messages/13631.cc
===================================================================
--- testsuite/22_locale/messages/13631.cc	(revision 0)
+++ testsuite/22_locale/messages/13631.cc	(revision 0)
@@ -0,0 +1,61 @@
+// { dg-require-namedlocale "fr_FR" }
+
+// Copyright (C) 2012 Free Software Foundation
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <locale>
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  bool test __attribute__((unused)) = true;
+  // This is defined through CXXFLAGS in scripts/testsuite_flags[.in].
+  const char* dir = LOCALEDIR;
+
+  std::locale l("fr_FR");
+
+  typedef std::messages<char> messages;
+
+  const messages &msgs_facet = std::use_facet<messages>(l);
+
+  messages::catalog msgs = msgs_facet.open("libstdc++", l, dir);
+  VERIFY( msgs >= 0 );
+
+  const char msgid[] = "please";
+  std::string translation1 = msgs_facet.get(msgs, 0, 0, msgid);
+
+  // Without a real translation this test doesn't mean anything:
+  VERIFY( translation1 != msgid );
+
+  // Opening an other catalog was enough to show the problem, even a fake
+  // catalog.
+  messages::catalog fake_msgs = msgs_facet.open("fake", l);
+
+  std::string translation2 = msgs_facet.get(msgs, 0, 0, msgid);
+
+  // Close catalogs before doing the check to avoid leaks.
+  msgs_facet.close(fake_msgs);
+  msgs_facet.close(msgs);
+
+  VERIFY( translation1 == translation2 );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}