diff mbox

[libstdc++/64584,libstdc++/64585] Clear basic_regex after imbue and make assign exception tolerant

Message ID CAG4ZjNmMbv4c-bix8VZV8CrudrZZOgMas3cKPhLwoiTnPOMUCA@mail.gmail.com
State New
Headers show

Commit Message

Tim Shen Jan. 15, 2015, 4:53 a.m. UTC
It's irony that I spent non-trivial effort to make sure basic_regex
still works after imbuing. :) But now with this specification, the
implementation can be cleaner (e.g. _M_original_str is removed).

Bootstrapped and tested.

I'll make a 4.9 patch later.

Thanks!

Comments

Jonathan Wakely Jan. 16, 2015, 12:12 p.m. UTC | #1
>@@ -516,14 +509,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>       template<typename _FwdIter>
> 	basic_regex(_FwdIter __first, _FwdIter __last,
> 		    flag_type __f = ECMAScript)
>-	: _M_flags(__f),
>-	  _M_loc(),
>-	  _M_original_str(__first, __last),
>-	  _M_automaton(__detail::__compile_nfa<_Rx_traits>(
>-	    _M_original_str.c_str(),
>-	    _M_original_str.c_str() + _M_original_str.size(),
>-	    _M_loc,
>-	    _M_flags))
>+	: basic_regex(std::move(__first), std::move(__last), locale_type(), __f)

Am I missing something here, why are you moving the iterators?

Iterators are cheap to copy, so moving them is not necessary.

>@@ -764,7 +745,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> #endif
>
>     private:
>-      typedef std::shared_ptr<__detail::_NFA<_Rx_traits>> _AutomatonPtr;
>+      typedef std::shared_ptr<const __detail::_NFA<_Rx_traits>> _AutomatonPtr;
>+
>+      template<typename _FwdIter>
>+	basic_regex(_FwdIter __first, _FwdIter __last, locale_type __loc,
>+		    flag_type __f)
>+	: _M_flags(__f), _M_loc(std::move(__loc)),
>+	_M_automaton(__detail::__compile_nfa<_FwdIter, _Rx_traits>(
>+	  std::move(__first), std::move(__last), _M_loc, _M_flags))

Again, I don't think std::move is needed for the iterators.

>diff --git a/libstdc++-v3/include/bits/regex_compiler.h b/libstdc++-v3/include/bits/regex_compiler.h
>index 81f8c8e..56962a9 100644
>--- a/libstdc++-v3/include/bits/regex_compiler.h
>+++ b/libstdc++-v3/include/bits/regex_compiler.h
>@@ -59,9 +59,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       _Compiler(_IterT __b, _IterT __e,
> 		const typename _TraitsT::locale_type& __traits, _FlagT __flags);
>
>-      std::shared_ptr<_RegexT>
>+      shared_ptr<const _RegexT>
>       _M_get_nfa()
>-      { return std::move(_M_nfa); }
>+      { return shared_ptr<const _RegexT>(_M_nfa.release()); }

This could be:

      { return shared_ptr<const _RegexT>(std::move(_M_nfa)); }

or simply left as it was:

      { return std::move(_M_nfa); }

because shared_ptr has an implicit conversion from unique_ptr rvalues.

>@@ -138,20 +138,65 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>       _FlagT              _M_flags;
>       _ScannerT           _M_scanner;
>-      shared_ptr<_RegexT> _M_nfa;
>+      unique_ptr<_RegexT> _M_nfa;
>       _StringT            _M_value;
>       _StackT             _M_stack;
>       const _TraitsT&     _M_traits;
>       const _CtypeT&      _M_ctype;
>     };
>diff --git a/libstdc++-v3/include/bits/regex_compiler.tcc b/libstdc++-v3/include/bits/regex_compiler.tcc
>index 33d7118..20b3172 100644
>--- a/libstdc++-v3/include/bits/regex_compiler.tcc
>+++ b/libstdc++-v3/include/bits/regex_compiler.tcc
>@@ -73,7 +73,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	       ? __flags
> 	       : __flags | regex_constants::ECMAScript),
>       _M_scanner(__b, __e, _M_flags, __loc),
>-      _M_nfa(make_shared<_RegexT>(__loc, _M_flags)),
>+      _M_nfa(new _RegexT(__loc, _M_flags)),

Am I right in thinking that this unique_ptr will always end up being
converted to a shared_ptr?

In that case, why bother using a unique_ptr initially, if it will just
have to allocate a shared_ptr control block later anyway? It's better
to use make_shared to reduce the number of allocations, isn't it?
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/regex.h b/libstdc++-v3/include/bits/regex.h
index 52c2384..6de883a 100644
--- a/libstdc++-v3/include/bits/regex.h
+++ b/libstdc++-v3/include/bits/regex.h
@@ -62,13 +62,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename, typename, typename, bool>
     class _Executor;
 
-  template<typename _TraitsT>
-    inline std::shared_ptr<_NFA<_TraitsT>>
-    __compile_nfa(const typename _TraitsT::char_type* __first,
-		  const typename _TraitsT::char_type* __last,
-		  const typename _TraitsT::locale_type& __loc,
-		  regex_constants::syntax_option_type __flags);
-
 _GLIBCXX_END_NAMESPACE_VERSION
 }
 
@@ -433,7 +426,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
        * character sequence.
        */
       basic_regex()
-      : _M_flags(ECMAScript), _M_loc(), _M_original_str(), _M_automaton(nullptr)
+      : _M_flags(ECMAScript), _M_loc(), _M_automaton(nullptr)
       { }
 
       /**
@@ -497,7 +490,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
 	basic_regex(const std::basic_string<_Ch_type, _Ch_traits,
 					    _Ch_alloc>& __s,
 		    flag_type __f = ECMAScript)
-	: basic_regex(__s.begin(), __s.end(), __f)
+	: basic_regex(__s.data(), __s.data() + __s.size(), __f)
 	{ }
 
       /**
@@ -516,14 +509,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       template<typename _FwdIter>
 	basic_regex(_FwdIter __first, _FwdIter __last,
 		    flag_type __f = ECMAScript)
-	: _M_flags(__f),
-	  _M_loc(),
-	  _M_original_str(__first, __last),
-	  _M_automaton(__detail::__compile_nfa<_Rx_traits>(
-	    _M_original_str.c_str(),
-	    _M_original_str.c_str() + _M_original_str.size(),
-	    _M_loc,
-	    _M_flags))
+	: basic_regex(std::move(__first), std::move(__last), locale_type(), __f)
 	{ }
 
       /**
@@ -657,15 +643,8 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
 	assign(const basic_string<_Ch_type, _Ch_traits, _Alloc>& __s,
 	       flag_type __flags = ECMAScript)
 	{
-	  _M_flags = __flags;
-	  _M_original_str.assign(__s.begin(), __s.end());
-	  auto __p = _M_original_str.c_str();
-	  _M_automaton = __detail::__compile_nfa<_Rx_traits>(
-	    __p,
-	    __p + _M_original_str.size(),
-	    _M_loc,
-	    _M_flags);
-	  return *this;
+	  return this->assign(basic_regex(__s.data(), __s.data() + __s.size(),
+					  _M_loc, _M_flags));
 	}
 
       /**
@@ -709,7 +688,11 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       unsigned int
       mark_count() const
-      { return _M_automaton->_M_sub_count() - 1; }
+      {
+	if (_M_automaton)
+	  return _M_automaton->_M_sub_count() - 1;
+	return 0;
+      }
 
       /**
        * @brief Gets the flags used to construct the regular expression
@@ -729,8 +712,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       imbue(locale_type __loc)
       {
 	std::swap(__loc, _M_loc);
-	if (_M_automaton != nullptr)
-	  this->assign(_M_original_str, _M_flags);
+	_M_automaton = nullptr;
 	return __loc;
       }
 
@@ -753,7 +735,6 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       {
 	std::swap(_M_flags, __rhs._M_flags);
 	std::swap(_M_loc, __rhs._M_loc);
-	std::swap(_M_original_str, __rhs._M_original_str);
 	std::swap(_M_automaton, __rhs._M_automaton);
       }
 
@@ -764,7 +745,15 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
 #endif
 
     private:
-      typedef std::shared_ptr<__detail::_NFA<_Rx_traits>> _AutomatonPtr;
+      typedef std::shared_ptr<const __detail::_NFA<_Rx_traits>> _AutomatonPtr;
+
+      template<typename _FwdIter>
+	basic_regex(_FwdIter __first, _FwdIter __last, locale_type __loc,
+		    flag_type __f)
+	: _M_flags(__f), _M_loc(std::move(__loc)),
+	_M_automaton(__detail::__compile_nfa<_FwdIter, _Rx_traits>(
+	  std::move(__first), std::move(__last), _M_loc, _M_flags))
+	{ }
 
       template<typename _Bp, typename _Ap, typename _Cp, typename _Rp,
 	__detail::_RegexExecutorPolicy, bool>
@@ -778,7 +767,6 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
 
       flag_type              _M_flags;
       locale_type            _M_loc;
-      basic_string<_Ch_type> _M_original_str;
       _AutomatonPtr          _M_automaton;
     };
 
diff --git a/libstdc++-v3/include/bits/regex_compiler.h b/libstdc++-v3/include/bits/regex_compiler.h
index 81f8c8e..56962a9 100644
--- a/libstdc++-v3/include/bits/regex_compiler.h
+++ b/libstdc++-v3/include/bits/regex_compiler.h
@@ -59,9 +59,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Compiler(_IterT __b, _IterT __e,
 		const typename _TraitsT::locale_type& __traits, _FlagT __flags);
 
-      std::shared_ptr<_RegexT>
+      shared_ptr<const _RegexT>
       _M_get_nfa()
-      { return std::move(_M_nfa); }
+      { return shared_ptr<const _RegexT>(_M_nfa.release()); }
 
     private:
       typedef _Scanner<_CharT>               _ScannerT;
@@ -138,20 +138,65 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       _FlagT              _M_flags;
       _ScannerT           _M_scanner;
-      shared_ptr<_RegexT> _M_nfa;
+      unique_ptr<_RegexT> _M_nfa;
       _StringT            _M_value;
       _StackT             _M_stack;
       const _TraitsT&     _M_traits;
       const _CtypeT&      _M_ctype;
     };
 
-  template<typename _TraitsT>
-    inline std::shared_ptr<_NFA<_TraitsT>>
-    __compile_nfa(const typename _TraitsT::char_type* __first,
-		  const typename _TraitsT::char_type* __last,
+  template<typename _Tp>
+    struct __has_contiguous_iter : std::false_type { };
+
+  template<typename _Ch, typename _Tr, typename _Alloc>
+    struct __has_contiguous_iter<std::basic_string<_Ch, _Tr, _Alloc>>
+    : std::true_type
+    { };
+
+  template<typename _Tp, typename _Alloc>
+    struct __has_contiguous_iter<std::vector<_Tp, _Alloc>>
+    : std::true_type
+    { };
+
+  template<typename _Tp>
+    struct __is_contiguous_normal_iter : std::false_type { };
+
+  template<typename _Tp, typename _Cont>
+    struct
+    __is_contiguous_normal_iter<__gnu_cxx::__normal_iterator<_Tp, _Cont>>
+    : __has_contiguous_iter<_Cont>::type
+    { };
+
+  template<typename _Iter, typename _TraitsT>
+    using __enable_if_contiguous_normal_iter
+      = typename enable_if< __is_contiguous_normal_iter<_Iter>::value,
+                           std::shared_ptr<const _NFA<_TraitsT>> >::type;
+
+  template<typename _Iter, typename _TraitsT>
+    using __disable_if_contiguous_normal_iter
+      = typename enable_if< !__is_contiguous_normal_iter<_Iter>::value,
+                           std::shared_ptr<const _NFA<_TraitsT>> >::type;
+
+  template<typename _FwdIter, typename _TraitsT>
+    inline __disable_if_contiguous_normal_iter<_FwdIter, _TraitsT>
+    __compile_nfa(_FwdIter __first, _FwdIter __last,
+		  const typename _TraitsT::locale_type& __loc,
+		  regex_constants::syntax_option_type __flags)
+    {
+      basic_string<typename _TraitsT::char_type> __str(__first, __last);
+      using _Cmplr = _Compiler<_TraitsT>;
+      return _Cmplr(__str.data(), __str.data() + __str.size(), __loc,
+		    __flags)._M_get_nfa();
+    }
+
+  template<typename _FwdIter, typename _TraitsT>
+    inline __enable_if_contiguous_normal_iter<_FwdIter, _TraitsT>
+    __compile_nfa(_FwdIter __first, _FwdIter __last,
 		  const typename _TraitsT::locale_type& __loc,
 		  regex_constants::syntax_option_type __flags)
     {
+      size_t __len = __last - __first;
+      const auto* __cfirst = __len ? std::__addressof(*__first) : nullptr;
       using _Cmplr = _Compiler<_TraitsT>;
       return _Cmplr(__first, __last, __loc, __flags)._M_get_nfa();
     }
diff --git a/libstdc++-v3/include/bits/regex_compiler.tcc b/libstdc++-v3/include/bits/regex_compiler.tcc
index 33d7118..20b3172 100644
--- a/libstdc++-v3/include/bits/regex_compiler.tcc
+++ b/libstdc++-v3/include/bits/regex_compiler.tcc
@@ -73,7 +73,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	       ? __flags
 	       : __flags | regex_constants::ECMAScript),
       _M_scanner(__b, __e, _M_flags, __loc),
-      _M_nfa(make_shared<_RegexT>(__loc, _M_flags)),
+      _M_nfa(new _RegexT(__loc, _M_flags)),
       _M_traits(_M_nfa->_M_traits),
       _M_ctype(std::use_facet<_CtypeT>(__loc))
     {
diff --git a/libstdc++-v3/include/std/regex b/libstdc++-v3/include/std/regex
index 4d0e93c..3dff372 100644
--- a/libstdc++-v3/include/std/regex
+++ b/libstdc++-v3/include/std/regex
@@ -56,9 +56,9 @@ 
 #include <bits/regex_constants.h>
 #include <bits/regex_error.h>
 #include <bits/regex_automaton.h>
-#include <bits/regex.h>
 #include <bits/regex_scanner.h>
 #include <bits/regex_compiler.h>
+#include <bits/regex.h>
 #include <bits/regex_executor.h>
 
 #endif // C++11
diff --git a/libstdc++-v3/testsuite/28_regex/basic_regex/assign/char/string.cc b/libstdc++-v3/testsuite/28_regex/basic_regex/assign/char/string.cc
index e0110a1..ee115b5 100644
--- a/libstdc++-v3/testsuite/28_regex/basic_regex/assign/char/string.cc
+++ b/libstdc++-v3/testsuite/28_regex/basic_regex/assign/char/string.cc
@@ -1,4 +1,3 @@ 
-// { dg-do compile }
 // { dg-options "-std=gnu++11" }
 
 // 2007-03-12  Stephen M. Webb  <stephen.webb@bregmasoft.com>
@@ -29,6 +28,7 @@ 
 // Tests C++ string assignment of the basic_regex class.  
 void test01()
 {
+  bool test __attribute__((unused)) = true;
   typedef std::basic_regex<char> test_type;
 
   std::string s("a*b");
@@ -36,9 +36,27 @@  void test01()
   re.assign(s);
 }
 
+// libstdc++/64584
+void test02()
+{
+  bool test __attribute__((unused)) = true;
+  std::regex re("", std::regex_constants::extended);
+  auto flags = re.flags();
+  try
+    {
+      re.assign("(", std::regex_constants::icase);
+      VERIFY(false);
+    }
+  catch (const std::regex_error& e)
+    {
+      VERIFY(flags == re.flags());
+    }
+}
+
 int
 main()
 { 
   test01();
+  test02();
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/28_regex/basic_regex/imbue/string.cc b/libstdc++-v3/testsuite/28_regex/basic_regex/imbue/string.cc
new file mode 100644
index 0000000..d4d4f47
--- /dev/null
+++ b/libstdc++-v3/testsuite/28_regex/basic_regex/imbue/string.cc
@@ -0,0 +1,44 @@ 
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2015 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
+// 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/>.
+
+// [28.8.5] class template basic_regex locale
+
+#include <string>
+#include <regex>
+#include <testsuite_hooks.h>
+
+// libstdc++/64585
+void test01()
+{
+  bool test __attribute__((unused)) = true;
+
+  static const char s[] = "a";
+  std::regex re("a");
+  VERIFY(std::regex_search(s, re));
+
+  auto loc = re.imbue(re.getloc());
+  VERIFY(!std::regex_search(s, re));
+}
+
+int
+main()
+{
+  test01();
+  return 0;
+}