Re: PR 13631 Problems in messages

[Jonathan Wakely <jwakely at redhat dot com>]

On 27/11/14 23:19 +0100, François Dumont wrote:
> Hi
>
>    Here is a revisited version. I finally go without compiling in 
> C++11 as I prefer to use __builtin_alloca instead of using 
> std::unique_ptr and heap memory. I also realized that I could use 
> codecvt::max_length to know the max number of multibytes.
>
>    Tested under Linux x86_64 with all locales installed. There are 
> some failures but not coming from this patch, will surely be the 
> subject of another mail.
>
> 2014-11-28  François Dumont  <fdumont@gcc.gnu.org>
>
>    DR libstdc++/13631
>    * include/bits/codecvt.h (__get_c_locale): New.
>    * 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 make catalog names to catalog ids.

s/make/map/

>    Add charset management.
>    * testsuite/22_locale/messages/13631.cc: New.
>    * testsuite/22_locale/messages/members/char/2.cc: Use also fr_FR locale
>    for charset conversion to get the expected accentuated character.

"accented"

> Do we need any abi tag ? Do we need to wait ?

I'll be posting my patch today, with ABI changes to the existing facets.

I'm not sure if this needs to be tagged though, you haven't added any
members or virtual functions to std::messages. The previously inline
functions were exported from the library already, because
messages<char> and messages<wchar_t> are explicitly instantiated.

I think with a few adjustments this can safely be applied to trunk
soon.


> Index: config/locale/gnu/messages_members.cc
> ===================================================================
> --- config/locale/gnu/messages_members.cc	(revision 218027)
> +++ config/locale/gnu/messages_members.cc	(working copy)
> @@ -31,54 +31,272 @@
> #include <locale>
> #include <bits/c++locale_internal.h>
>
> +#include <algorithm>
> +#include <utility>
> +
> +#include <ext/concurrence.h>
> +
> +namespace
> +{
> +  using namespace std;
> +
> +  class Catalogs
> +  {
> +  public:
> +    typedef messages_base::catalog catalog_id;
> +
> +    struct catalog_info
> +    {
> +      catalog_info()
> +      { }
> +
> +      catalog_info(catalog_id __id, const char* __domain, locale __loc)
> +	: _M_id(__id), _M_domain(__domain), _M_locale(__loc)
> +      { }
> +

I don't really like that this type doesn't own _M_domain and it has to
be freed by Catalog, but without a move constructor (which requires
C++11) it would be awkward to set _M_domain=0 after copy constructing
a new catalog_info.

So it's OK like this.

> +      catalog_id _M_id;
> +      const char* _M_domain;
> +      locale _M_locale;
> +    };
> +
> +    typedef pair<const char*, locale> result_type;
> +
> +    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]._M_domain;
> +	  delete[] _M_map;
> +	}
> +    }
> +
> +    catalog_id
> +    _M_add(const string& __domain, locale __l)
> +    {
> +      __gnu_cxx::__scoped_lock lock(_M_mutex);
> +
> +      catalog_info* __new_map = new catalog_info[_M_nb_entry + 1];

I wonder if we should grow this array by more than one element at a
time.

> +      __try
> +	{
> +	  copy(_M_map, _M_map + _M_nb_entry, __new_map);
> +	  char* __domain_copy = new char[__domain.size() + 1];
> +	  __domain.copy(__domain_copy, __domain.size());
> +	  __domain_copy[__domain.size()] = 0;
> +	  __new_map[_M_nb_entry] = catalog_info(_M_counter, __domain_copy, __l);
> +	}
> +      __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.

"opened/closed which is considered"

I think it's fine to say you can't have more than 2^31 open message
catalogs, but we should for numeric_limits<catalog>::max() before
doing anything and return -1 to say it couldn't be opened.

> +      catalog_id __cat = _M_counter++;
> +      delete[] _M_map;
> +      _M_map = __new_map;
> +      ++_M_nb_entry;
> +
> +      return __cat;
> +    }
> +
> +    void
> +    _M_erase(catalog_id __c)
> +    {
> +      __gnu_cxx::__scoped_lock lock(_M_mutex);
> +
> +      catalog_info* __entry =
> +	lower_bound(_M_map, _M_map + _M_nb_entry, __c, _Comp());
> +      if (__entry == _M_map + _M_nb_entry || __entry->_M_id != __c)
> +	return;
> +      
> +      catalog_info* __new_map =
> +	_M_nb_entry > 1 ? new catalog_info[_M_nb_entry - 1] : 0;

Is it really necessary to shrink the map again?

I would expect that opening and closing several message catalogs in
succession would not need to keep reallocating the map.

You're also creating, copying and destroying std::locale objects every
time you reallocate the map, which generates a small storm of atomic
operations on the reference counts for the locales.

One option would be to create an array of char and fill it with
std::copy_uninitialized instead of std::copy. Or just use std::vector.

> +      copy(__entry + 1, _M_map + _M_nb_entry,
> +	   copy(_M_map, __entry, __new_map));
> +
> +      delete[] __entry->_M_domain;
> +      delete[] _M_map;
> +      _M_map = __new_map;
> +      --_M_nb_entry;
> +    }
> +
> +    result_type
> +    _M_get(catalog_id __c) const
> +    {
> +      __gnu_cxx::__scoped_lock lock(_M_mutex);
> +
> +      const catalog_info* __entry =
> +	lower_bound(_M_map, _M_map + _M_nb_entry, __c, _Comp());
> +      if (__entry != _M_map + _M_nb_entry && __entry->_M_id == __c)
> +	return result_type(__entry->_M_domain, __entry->_M_locale);
> +      return result_type(0, locale());

I assume the second return was just for testing, it should be removed.

> namespace std _GLIBCXX_VISIBILITY(default)
> {
> -_GLIBCXX_BEGIN_NAMESPACE_VERSION
> +  _GLIBCXX_BEGIN_NAMESPACE_VERSION

This re-indenting is incorrect.

> Index: config/locale/gnu/messages_members.h
> ===================================================================
> --- config/locale/gnu/messages_members.h	(revision 218027)
> +++ config/locale/gnu/messages_members.h	(working copy)
> @@ -83,22 +83,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)

Unless I'm misreading this patch, you've removed the definitions of
messages<_CharT>::do_open() and messages<_CharT>::do_close() for the
primary template. They would stil be needed if users instantiate e.g.
messages<char16_t> or messages<signed char>.


> Index: include/bits/codecvt.h
> ===================================================================
> --- include/bits/codecvt.h	(revision 218027)
> +++ include/bits/codecvt.h	(working copy)
> @@ -390,6 +388,10 @@
>
>       virtual int
>       do_max_length() const throw();
> +
> +      friend __c_locale
> +      __get_c_locale(const codecvt& __codecvt)
> +      { return __codecvt._M_c_locale_codecvt; }
>   };
>
> #ifdef _GLIBCXX_USE_WCHAR_T
> @@ -450,6 +452,10 @@
>
>       virtual int
>       do_max_length() const throw();
> +
> +      friend __c_locale
> +      __get_c_locale(const codecvt& __codecvt)
> +      { return __codecvt._M_c_locale_codecvt; }
>     };
> #endif //_GLIBCXX_USE_WCHAR_T

There seems no point making these friend functions, since anyone can
call those functions, you might as well just have a public
_M_get_c_locale() member.

However, could it just be friend class messages<char> ?


> Index: testsuite/22_locale/messages/13631.cc
> ===================================================================
> --- testsuite/22_locale/messages/13631.cc	(revision 0)
> +++ testsuite/22_locale/messages/13631.cc	(working copy)
> @@ -0,0 +1,99 @@
> +// { dg-require-namedlocale "fr_FR" }
> +
> +// Copyright (C) 2014 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.

"another"

> +  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 );
> +}
> +
> +void test02()
> +{
> +  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<wchar_t> messages;
> +
> +  const messages &msgs_facet = std::use_facet<messages>(l);
> +
> +  messages::catalog msgs = msgs_facet.open("libstdc++", l, dir);
> +  VERIFY( msgs >= 0 );
> +
> +  const wchar_t msgid[] = L"please";
> +  std::wstring translation1 = msgs_facet.get(msgs, 0, 0, msgid);
> +
> +  // Without a real translation this test doesn't mean anything:
> +  VERIFY( !translation1.empty() );
> +  VERIFY( translation1 != msgid );
> +
> +  // Opening an other catalog was enough to show the problem, even a fake

"another"