This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Some std::locale improvements


I think we also need this to make __numpunct_cache and
__moneypunct_cache exception-safe. If we set _M_allocated=true at the
start of the _M_cache functions and then an allocation throws we will
delete[] the memory allocated in _M_cache, but then the cache's
destructor will see _M_allocated==true and will try to delete[] them
again:

 template<typename _CharT>
   __numpunct_cache<_CharT>::~__numpunct_cache()
   {
     if (_M_allocated)
       {
         delete [] _M_grouping;
         delete [] _M_truename;
         delete [] _M_falsename;
       }
   }

Delaying setting the bool and the pointers themselves until after all
allocations avoids that.

The _M_cache functions were also calling virtual functions twice when
once is enough.

Finally, __timepunct::_M_cache() is declared but never defined, so I
want to remove that.

Tested x86_64-linux and powerpc64-linux.

Does anyone see anything wrong with my reasoning above before I commit
this?

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index d9b11b5..72a4020 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -2,6 +2,14 @@
 
 	* config/abi/pre/gnu.ver: Fix ios_base::failure exports.
 
+	* include/bits/locale_facets.h (numpunct::_M_cache): Avoid calling
+	virtual functions twice. Only update _M_allocated after all
+	allocations have succeeded.
+	* include/bits/locale_facets_nonio.tcc (moneypunct::_M_cache):
+	Likewise.
+	* include/bits/locale_facets_nonio.h (__timepunct::_M_cache): Remove
+	unused declaration.
+
 2014-11-29  Jonathan Wakely  <jwakely@redhat.com>
 
 	* include/bits/locale_facets/nonio.h (__timepunct): Remove unused
diff --git a/libstdc++-v3/include/bits/locale_facets.tcc b/libstdc++-v3/include/bits/locale_facets.tcc
index 88adc0d..200e099 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -77,8 +77,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     __numpunct_cache<_CharT>::_M_cache(const locale& __loc)
     {
-      _M_allocated = true;
-
       const numpunct<_CharT>& __np = use_facet<numpunct<_CharT> >(__loc);
 
       char* __grouping = 0;
@@ -86,24 +84,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _CharT* __falsename = 0;
       __try
 	{
-	  _M_grouping_size = __np.grouping().size();
+	  const string& __g = __np.grouping();
+	  _M_grouping_size = __g.size();
 	  __grouping = new char[_M_grouping_size];
-	  __np.grouping().copy(__grouping, _M_grouping_size);
-	  _M_grouping = __grouping;
+	  __g.copy(__grouping, _M_grouping_size);
 	  _M_use_grouping = (_M_grouping_size
-			     && static_cast<signed char>(_M_grouping[0]) > 0
-			     && (_M_grouping[0]
+			     && static_cast<signed char>(__grouping[0]) > 0
+			     && (__grouping[0]
 				 != __gnu_cxx::__numeric_traits<char>::__max));
 
-	  _M_truename_size = __np.truename().size();
+	  const basic_string<_CharT>& __tn = __np.truename();
+	  _M_truename_size = __tn.size();
 	  __truename = new _CharT[_M_truename_size];
-	  __np.truename().copy(__truename, _M_truename_size);
-	  _M_truename = __truename;
+	  __tn.copy(__truename, _M_truename_size);
 
-	  _M_falsename_size = __np.falsename().size();
+	  const basic_string<_CharT>& __fn = __np.falsename();
+	  _M_falsename_size = __fn.size();
 	  __falsename = new _CharT[_M_falsename_size];
-	  __np.falsename().copy(__falsename, _M_falsename_size);
-	  _M_falsename = __falsename;
+	  __fn.copy(__falsename, _M_falsename_size);
 
 	  _M_decimal_point = __np.decimal_point();
 	  _M_thousands_sep = __np.thousands_sep();
@@ -115,6 +113,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  __ct.widen(__num_base::_S_atoms_in,
 		     __num_base::_S_atoms_in
 		     + __num_base::_S_iend, _M_atoms_in);
+
+	  _M_grouping = __grouping;
+	  _M_truename = __truename;
+	  _M_falsename = __falsename;
+	  _M_allocated = true;
 	}
       __catch(...)
 	{
diff --git a/libstdc++-v3/include/bits/locale_facets_nonio.h b/libstdc++-v3/include/bits/locale_facets_nonio.h
index 5c1eeb7..1b0aff9 100644
--- a/libstdc++-v3/include/bits/locale_facets_nonio.h
+++ b/libstdc++-v3/include/bits/locale_facets_nonio.h
@@ -138,9 +138,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ~__timepunct_cache();
 
-      void
-      _M_cache(const locale& __loc);
-
     private:
       __timepunct_cache&
       operator=(const __timepunct_cache&);
diff --git a/libstdc++-v3/include/bits/locale_facets_nonio.tcc b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
index c9f8dac..42c3504 100644
--- a/libstdc++-v3/include/bits/locale_facets_nonio.tcc
+++ b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
@@ -68,8 +68,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     __moneypunct_cache<_CharT, _Intl>::_M_cache(const locale& __loc)
     {
-      _M_allocated = true;
-
       const moneypunct<_CharT, _Intl>& __mp =
 	use_facet<moneypunct<_CharT, _Intl> >(__loc);
 
@@ -83,29 +81,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _CharT* __negative_sign = 0;     
       __try
 	{
-	  _M_grouping_size = __mp.grouping().size();
+	  const string& __g = __mp.grouping();
+	  _M_grouping_size = __g.size();
 	  __grouping = new char[_M_grouping_size];
-	  __mp.grouping().copy(__grouping, _M_grouping_size);
-	  _M_grouping = __grouping;
+	  __g.copy(__grouping, _M_grouping_size);
 	  _M_use_grouping = (_M_grouping_size
-			     && static_cast<signed char>(_M_grouping[0]) > 0
-			     && (_M_grouping[0]
+			     && static_cast<signed char>(__grouping[0]) > 0
+			     && (__grouping[0]
 				 != __gnu_cxx::__numeric_traits<char>::__max));
 
-	  _M_curr_symbol_size = __mp.curr_symbol().size();
+	  const basic_string<_CharT>& __cs = __mp.curr_symbol();
+	  _M_curr_symbol_size = __cs.size();
 	  __curr_symbol = new _CharT[_M_curr_symbol_size];
-	  __mp.curr_symbol().copy(__curr_symbol, _M_curr_symbol_size);
-	  _M_curr_symbol = __curr_symbol;
+	  __cs.copy(__curr_symbol, _M_curr_symbol_size);
 
-	  _M_positive_sign_size = __mp.positive_sign().size();
+	  const basic_string<_CharT>& __ps = __mp.positive_sign();
+	  _M_positive_sign_size = __ps.size();
 	  __positive_sign = new _CharT[_M_positive_sign_size];
-	  __mp.positive_sign().copy(__positive_sign, _M_positive_sign_size);
-	  _M_positive_sign = __positive_sign;
+	  __ps.copy(__positive_sign, _M_positive_sign_size);
 
-	  _M_negative_sign_size = __mp.negative_sign().size();
+	  const basic_string<_CharT>& __ns = __mp.negative_sign();
+	  _M_negative_sign_size = __ns.size();
 	  __negative_sign = new _CharT[_M_negative_sign_size];
-	  __mp.negative_sign().copy(__negative_sign, _M_negative_sign_size);
-	  _M_negative_sign = __negative_sign;
+	  __ns.copy(__negative_sign, _M_negative_sign_size);
 
 	  _M_pos_format = __mp.pos_format();
 	  _M_neg_format = __mp.neg_format();
@@ -113,6 +111,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  const ctype<_CharT>& __ct = use_facet<ctype<_CharT> >(__loc);
 	  __ct.widen(money_base::_S_atoms,
 		     money_base::_S_atoms + money_base::_S_end, _M_atoms);
+
+	  _M_grouping = __grouping;
+	  _M_curr_symbol = __curr_symbol;
+	  _M_positive_sign = __positive_sign;
+	  _M_negative_sign = __negative_sign;
+	  _M_allocated = true;
 	}
       __catch(...)
 	{

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]