Here is what I have finally commited to libstdcxx_so_7-2 branch.
2012-03-01 François Dumont <fdum...@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;
+}
