Re: [so_7-2] DR 13631 patch

[Paolo Carlini <paolo dot carlini at oracle dot com>]

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.