From patchwork Thu Mar 1 20:43:04 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Dumont?= X-Patchwork-Id: 144111 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 54AECB6EE8 for ; Fri, 2 Mar 2012 07:43:43 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1331239424; h=Comment: DomainKey-Signature:Received:Received:Received:Received: Received-SPF:Received:Received:Received:Message-ID:Date:From: User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=P5TnM09 HKnxg1/xdq13kAP2HzZc=; b=gY4w85cyM/MlrWDUd/54ue1RnpN5H1ebPtnSbd1 Nd5ks8e/ZvsdEfs7j5wDDQ2F5pGRYPBwoETII/7RmYdf3vWm6lvIamqHfXh7v3Hc qmz+LBgOfQCILdPSnibKOOwkJU8yy7EiFUzy2o0ux1AMJX0ec/kFZj2xGkPVjl/j uAis= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received-SPF:Authentication-Results:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=L6j60l1Mh4Gltc/6KTpqqoZaqvOMRwYjpXABc9w7aS8wWQqNclDkPmpn2SQXyF 0d5Fq0KH5mBNKT4RRgTzE7pmTWsYZIB5ECDNtUQ+yiDBjF30tVXPzbeYd8cZuNi+ j7ek9EiS95NeP/I4YQmDrEOWGA+Sk2oWo5PDgI7nHZIfs=; Received: (qmail 11007 invoked by alias); 1 Mar 2012 20:43:34 -0000 Received: (qmail 10623 invoked by uid 22791); 1 Mar 2012 20:43:27 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, TW_CX, TW_DC X-Spam-Check-By: sourceware.org Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Mar 2012 20:43:10 +0000 Received: by wibhq12 with SMTP id hq12so245178wib.20 for ; Thu, 01 Mar 2012 12:43:07 -0800 (PST) Received-SPF: pass (google.com: domain of frs.dumont@gmail.com designates 10.180.99.65 as permitted sender) client-ip=10.180.99.65; Authentication-Results: mr.google.com; spf=pass (google.com: domain of frs.dumont@gmail.com designates 10.180.99.65 as permitted sender) smtp.mail=frs.dumont@gmail.com; dkim=pass header.i=frs.dumont@gmail.com Received: from mr.google.com ([10.180.99.65]) by 10.180.99.65 with SMTP id eo1mr5923169wib.13.1330634587379 (num_hops = 1); Thu, 01 Mar 2012 12:43:07 -0800 (PST) Received: by 10.180.99.65 with SMTP id eo1mr4778012wib.13.1330634587291; Thu, 01 Mar 2012 12:43:07 -0800 (PST) Received: from localhost.localdomain (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by mx.google.com with ESMTPS id be4sm15230871wib.8.2012.03.01.12.43.05 (version=SSLv3 cipher=OTHER); Thu, 01 Mar 2012 12:43:05 -0800 (PST) Message-ID: <4F4FDF58.2000209@gmail.com> Date: Thu, 01 Mar 2012 21:43:04 +0100 From: =?ISO-8859-1?Q?Fran=E7ois_Dumont?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20111109 Thunderbird/8.0 MIME-Version: 1.0 To: Paolo Carlini CC: "libstdc++@gcc.gnu.org" , gcc-patches@gcc.gnu.org Subject: Re: [so_7-2] DR 13631 patch References: <4F414F0B.8040400@gmail.com> <4F44D1BD.8020303@oracle.com> <4F455529.5080509@gmail.com> <167E5F01-2F44-467A-854B-2A8CB2E3005D@oracle.com> <4F4D3576.3090403@gmail.com> <4F4E05D6.7030705@oracle.com> In-Reply-To: <4F4E05D6.7030705@oracle.com> Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Here is what I have finally commited to libstdcxx_so_7-2 branch. 2012-03-01 François Dumont 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 to pair. 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 >> +//. >> + >> +#include >> +#include >> + >> +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 messages; >> + >> + const messages&msgs_facet = std::use_facet(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 >> #include >> >> +#include >> +#include >> +#include >> + >> +namespace >> +{ >> + using namespace std; >> + >> + struct Comp >> + { >> + typedef messages_base::catalog catalog; >> + typedef pair _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 _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 #include +#include +#include +#include + +namespace +{ + using namespace std; + + typedef messages_base::catalog catalog; + typedef pair _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 _M_get(catalog __c) const + { + __gnu_cxx::__scoped_lock lock(_M_mutex); + + typedef pair _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 __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(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::catalog + messages::do_open(const basic_string& __s, + const locale&) const + { return get_catalogs()._M_add(__s); } + + template<> + void + messages::do_close(catalog __c) const + { get_catalogs()._M_erase(__c); } + + template<> string - messages::do_get(catalog, int, int, const string& __dfault) const + messages::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(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::catalog + messages::do_open(const basic_string& __s, + const locale&) const + { return get_catalogs()._M_add(__s); } + + template<> + void + messages::do_close(catalog __c) const + { get_catalogs()._M_erase(__c); } + + template<> wstring - messages::do_get(catalog, int, int, const wstring& __dfault) const + messages::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(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 messages<_CharT>::catalog - messages<_CharT>::do_open(const basic_string& __s, - const locale&) const - { - // No error checking is done, assume the catalog exists and can - // be used. - textdomain(__s.c_str()); - return 0; - } - - template - void - messages<_CharT>::do_close(catalog) const - { } - // messages_byname template messages_byname<_CharT>::messages_byname(const char* __s, size_t __refs) @@ -127,5 +111,25 @@ } } + template<> + typename messages::catalog + messages::do_open(const basic_string&, + const locale&) const; + + template<> + void + messages::do_close(catalog) const; + +#ifdef _GLIBCXX_USE_WCHAR_T + template<> + typename messages::catalog + messages::do_open(const basic_string&, + const locale&) const; + + template<> + void + messages::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 +// . + +#include +#include + +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 messages; + + const messages &msgs_facet = std::use_facet(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; +}