PR 13631 Problems in messages

[Jonathan Wakely]

On 01/12/14 22:42 +0100, François Dumont wrote:
>Hi
>
>    Here is another proposal that consider all your remarks except 
>one. I finally prefer to go with std::vector of pointers. Dynamically 
>allocating Catalog_info allow to avoid numerous copies of locale when 
>we find this pointer from the catalog info.
>
>
>>+ 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.
>
>I don't get this one. I still need to return something when I can't 
>find the catalog so the 2 return statements.

I thought I remembered seeing two returns at the same level of
indentation, but obviously they are not! Maybe I was looking it it in
gmail's attachment viewer which eats leading whitespace. Ignore that
comment, sorry.

>>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>.
>
>Yes but do you confirm that it is already the same problem with do_get ?

Yes, I'm just wondering why make the situation worse than it already
is. Does leaving the default definitions cause a problem?

If some user has got code that uses messages<signed char> and they
provide a definition for messages<signed char>::do_get() their code
will break if do_open and do_close disappear. (Realistically I doubt
anyone is doing that though, it may not even work.)

>In my opinion we could provide template implementations of all those 
>methods relying on codecvt<_CharT, char, mbstate_t>, even for do_get. 
>But in this case some implementation details will be exposed in the 
>header files and additional symbols will have to be exported I think.
>
>In fact I have already started doing something like that but then 
>start facing issue accessing nl_langinfo_l. Shall I go further and 
>provide a patch doing this ?

IMHO no.

>+    catalog
>+    _M_add(const string& __domain, locale __l)
>+    {
>+      __gnu_cxx::__scoped_lock lock(_M_mutex);
>+
>+      // The counter is not likely to roll unless catalogs keep on being
>+      // opened/closed which is consider as an application mistake for the
>+      // moment.
>+      if (_M_catalog_counter == numeric_limits<catalog>::max())
>+	return -1;
>+
>+      _M_infos.push_back(new Catalog_info(_M_catalog_counter++, __domain, __l));
>+      return _M_infos.back()->_M_id;

This is not exception safe. You can use auto_ptr to fix it:

      std::auto_ptr<Catalog_info> info(new Catalog_info(_M_catalog_counter++,
                                                        __domain, __l));
      _M_infos.push_back(info.get());
      return info.release()->_M_id;