[so_7-2] DR 13631 patch

[Paolo Carlini]

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 function 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.