[so_7-2] DR 13631 patch

[François Dumont]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: message.patch
Type: text/x-patch
Size: 10951 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20120301/b97bc930/attachment.bin>