std::regex: inserting std::wregex to std::vector loses some std::wregex values

Tim Shen timshen@google.com
Sun Sep 14 18:13:00 GMT 2014


On Sun, Sep 14, 2014 at 3:16 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Thus, are you going to send an actual patch? I'm wondering if we could
> arrange it in such a way not to break the binary compatibility in the
> release branch (thus fix the bug there too) perhaps by not removing any data
> members, simply leaving them unused?! In mainline I suppose we are going to
> stabilize the ABI for the 5.0 release, thus we can do that. While we are at
> it, check that the order of the final remaining data members is still
> optimal size-wise.

I've made a patch that changes basic_regex's member only, by making
basic_regex::_M_traits heap allocated.

I believe that I didn't get it, but I'm still a little bit confused,
since we haven't instantiated and exported any regex class (except
regex_error, I suppose) to the binary, right?

Anyway, the patch is bootstrapped and tested with debug flag. Can it
be modified slightly to keep the binary compatibility?

Thanks :)


-- 
Regards,
Tim Shen
-------------- next part --------------
commit 174dfef5322948e83438be91672ca8c8ed0a9abf
Author: Tim Shen <timshen@google.com>
Date:   Fri Sep 12 22:25:21 2014 -0700

    	PR libstdc++/63199
    	* include/bits/regex.h (__compile_nfa, basic_regex::basic_regex,
    	basic_regex::assign, basic_regex::mark_count, basic_regex::imbue,
    	basic_regex::getloc, basic_regex::swap):
    	Make basic_regex::_M_traits heap allocated and
    	basic_regex::_M_automaton a value instead of a shared_ptr.
    	* include/bits/regex.tcc (__regex_algo_impl): Adjust for
    	_M_traits and _M_automaton changes.
    	* include/bits/regex_automaton.h (_NFA::operator=,
    	_NFA_base::operator=): Make _NFA move assignable.
    	* include/bits/regex_compiler.h (_Compiler::_M_get_nfa,
    	__compile_nfa): Adjust for _M_automaton change.
    	* include/bits/regex_executor.h (_Executor::_Executor,
    	_Executor::_M_is_word): Adjust for _M_traits change.
    	* include/bits/regex_executor.tcc (): Adjust for _M_traits change;
    	Fix the assertion that test ECMAScript in DFS mode.
    	* testsuite/28_regex/algorithms/regex_match/ecma/wchar_t/63199.cc:
    	New testcase.

diff --git a/libstdc++-v3/include/bits/regex.h b/libstdc++-v3/include/bits/regex.h
index 9dc83fd..f4cc92c 100644
--- a/libstdc++-v3/include/bits/regex.h
+++ b/libstdc++-v3/include/bits/regex.h
@@ -61,7 +61,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     class _Executor;
 
   template<typename _TraitsT>
-    inline std::shared_ptr<_NFA<_TraitsT>>
+    inline _NFA<_TraitsT>
     __compile_nfa(const typename _TraitsT::char_type* __first,
 		  const typename _TraitsT::char_type* __last,
 		  const _TraitsT& __traits,
@@ -433,7 +433,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        * character sequence.
        */
       basic_regex()
-      : _M_flags(ECMAScript), _M_automaton(nullptr)
+      : _M_flags(ECMAScript), _M_traits(new _Rx_traits()),
+        _M_automaton(_M_flags)
       { }
 
       /**
@@ -474,17 +475,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        *
        * @param __rhs A @p regex object.
        */
-      basic_regex(const basic_regex& __rhs) = default;
+      basic_regex(const basic_regex& __rhs)
+      : basic_regex(__rhs._M_original_str, __rhs._M_flags)
+      { }
 
       /**
        * @brief Move-constructs a basic regular expression.
        *
        * @param __rhs A @p regex object.
        */
-      basic_regex(const basic_regex&& __rhs) noexcept
-      : _M_flags(__rhs._M_flags), _M_traits(__rhs._M_traits),
-	_M_automaton(std::move(__rhs._M_automaton))
-      { }
+      basic_regex(basic_regex&& __rhs) noexcept = default;
 
       /**
        * @brief Constructs a basic regular expression from the string
@@ -520,11 +520,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	basic_regex(_FwdIter __first, _FwdIter __last,
 		    flag_type __f = ECMAScript)
 	: _M_flags(__f),
+	  _M_traits(new _Rx_traits()),
 	  _M_original_str(__first, __last),
 	  _M_automaton(__detail::__compile_nfa(_M_original_str.c_str(),
 					       _M_original_str.c_str()
 						 + _M_original_str.size(),
-					       _M_traits,
+					       *_M_traits,
 					       _M_flags))
 	{ }
 
@@ -664,7 +665,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  auto __p = _M_original_str.c_str();
 	  _M_automaton = __detail::__compile_nfa(__p,
 						 __p + _M_original_str.size(),
-						 _M_traits, _M_flags);
+						 *_M_traits, _M_flags);
 	  return *this;
 	}
 
@@ -709,7 +710,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        */
       unsigned int
       mark_count() const
-      { return _M_automaton->_M_sub_count() - 1; }
+      { return _M_automaton._M_sub_count() - 1; }
 
       /**
        * @brief Gets the flags used to construct the regular expression
@@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       locale_type
       imbue(locale_type __loc)
       {
-	auto __ret = _M_traits.imbue(__loc);
+	auto __ret = _M_traits->imbue(__loc);
 	this->assign(_M_original_str, _M_flags);
 	return __ret;
       }
@@ -739,7 +740,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        */
       locale_type
       getloc() const
-      { return _M_traits.getloc(); }
+      { return _M_traits->getloc(); }
 
       // [7.8.6] swap
       /**
@@ -752,18 +753,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	std::swap(_M_flags, __rhs._M_flags);
 	std::swap(_M_traits, __rhs._M_traits);
+	std::swap(_M_original_str, __rhs._M_original_str);
 	std::swap(_M_automaton, __rhs._M_automaton);
       }
 
 #ifdef _GLIBCXX_DEBUG
       void
       _M_dot(std::ostream& __ostr)
-      { _M_automaton->_M_dot(__ostr); }
+      { _M_automaton._M_dot(__ostr); }
 #endif
 
     protected:
-      typedef std::shared_ptr<__detail::_NFA<_Rx_traits>> _AutomatonPtr;
-
       template<typename _Bp, typename _Ap, typename _Cp, typename _Rp,
 	__detail::_RegexExecutorPolicy, bool>
 	friend bool
@@ -774,10 +774,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       template<typename, typename, typename, bool>
 	friend class __detail::_Executor;
 
-      flag_type              _M_flags;
-      _Rx_traits             _M_traits;
-      basic_string<_Ch_type> _M_original_str;
-      _AutomatonPtr          _M_automaton;
+      flag_type                  _M_flags;
+      unique_ptr<_Rx_traits>     _M_traits;
+      basic_string<_Ch_type>     _M_original_str;
+      __detail::_NFA<_Rx_traits> _M_automaton;
     };
 
   /** @brief Standard regular expressions. */
diff --git a/libstdc++-v3/include/bits/regex.tcc b/libstdc++-v3/include/bits/regex.tcc
index 3322379..db9bc5f 100644
--- a/libstdc++-v3/include/bits/regex.tcc
+++ b/libstdc++-v3/include/bits/regex.tcc
@@ -58,11 +58,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		      const basic_regex<_CharT, _TraitsT>& __re,
 		      regex_constants::match_flag_type     __flags)
     {
-      if (__re._M_automaton == nullptr)
+      if (__re._M_automaton.empty())
 	return false;
 
       typename match_results<_BiIter, _Alloc>::_Base_type& __res = __m;
-      __res.resize(__re._M_automaton->_M_sub_count() + 2);
+      __res.resize(__re._M_automaton._M_sub_count() + 2);
       for (auto& __it : __res)
 	__it.matched = false;
 
@@ -70,7 +70,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // without defining a macro. Users should define
       // _GLIBCXX_REGEX_USE_THOMPSON_NFA if they need to use this approach.
       bool __ret;
-      if (!__re._M_automaton->_M_has_backref
+      if (!__re._M_automaton._M_has_backref
 	  && !(__re._M_flags & regex_constants::ECMAScript)
 #ifndef _GLIBCXX_REGEX_USE_THOMPSON_NFA
 	  && __policy == _RegexExecutorPolicy::_S_alternate
diff --git a/libstdc++-v3/include/bits/regex_automaton.h b/libstdc++-v3/include/bits/regex_automaton.h
index 27ec671..dcd36e5 100644
--- a/libstdc++-v3/include/bits/regex_automaton.h
+++ b/libstdc++-v3/include/bits/regex_automaton.h
@@ -130,6 +130,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     _NFA_base(_NFA_base&&) = default;
 
+    _NFA_base& operator=(_NFA_base&&) = default;
+
   protected:
     ~_NFA_base() = default;
 
@@ -166,6 +168,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _NFA(const _NFA&) = delete;
       _NFA(_NFA&&) = default;
 
+      _NFA& operator=(const _NFA&) = delete;
+      _NFA& operator=(_NFA&&) = default;
+
       _StateIdT
       _M_insert_accept()
       {
diff --git a/libstdc++-v3/include/bits/regex_compiler.h b/libstdc++-v3/include/bits/regex_compiler.h
index 1193a5a..5ba7397 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 _TraitsT& __traits, _FlagT __flags);
 
-      std::shared_ptr<_RegexT>
+      _RegexT
       _M_get_nfa()
-      { return make_shared<_RegexT>(std::move(_M_nfa)); }
+      { return std::move(_M_nfa); }
 
     private:
       typedef _Scanner<_CharT>               _ScannerT;
@@ -145,7 +145,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     };
 
   template<typename _TraitsT>
-    inline std::shared_ptr<_NFA<_TraitsT>>
+    inline _NFA<_TraitsT>
     __compile_nfa(const typename _TraitsT::char_type* __first,
 		  const typename _TraitsT::char_type* __last,
 		  const _TraitsT& __traits,
diff --git a/libstdc++-v3/include/bits/regex_executor.h b/libstdc++-v3/include/bits/regex_executor.h
index 130bc74..b0a806d 100644
--- a/libstdc++-v3/include/bits/regex_executor.h
+++ b/libstdc++-v3/include/bits/regex_executor.h
@@ -74,7 +74,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_begin(__begin),
       _M_end(__end),
       _M_re(__re),
-      _M_nfa(*__re._M_automaton),
+      _M_nfa(__re._M_automaton),
       _M_results(__results),
       _M_rep_count(_M_nfa.size()),
       _M_states(_M_nfa._M_start(), _M_nfa.size()),
@@ -125,8 +125,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_is_word(_CharT __ch) const
       {
 	static const _CharT __s[2] = { 'w' };
-	return _M_re._M_traits.isctype
-	  (__ch, _M_re._M_traits.lookup_classname(__s, __s+1));
+	return _M_re._M_traits->isctype
+	  (__ch, _M_re._M_traits->lookup_classname(__s, __s+1));
       }
 
       bool
diff --git a/libstdc++-v3/include/bits/regex_executor.tcc b/libstdc++-v3/include/bits/regex_executor.tcc
index 3ca7de3..5120c6a 100644
--- a/libstdc++-v3/include/bits/regex_executor.tcc
+++ b/libstdc++-v3/include/bits/regex_executor.tcc
@@ -312,9 +312,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		 __last != _M_end && __tmp != __submatch.second;
 		 ++__tmp)
 	      ++__last;
-	    if (_M_re._M_traits.transform(__submatch.first,
-						__submatch.second)
-		== _M_re._M_traits.transform(_M_current, __last))
+	    if (_M_re._M_traits->transform(__submatch.first,
+					   __submatch.second)
+		== _M_re._M_traits->transform(_M_current, __last))
 	      {
 		if (__last != _M_current)
 		  {
@@ -380,8 +380,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	case _S_opcode_alternative:
 	  if (_M_nfa._M_flags & regex_constants::ECMAScript)
 	    {
-	      // TODO: Let DFS support ECMAScript's alternative operation.
-	      _GLIBCXX_DEBUG_ASSERT(!__dfs_mode);
+	      // TODO: Let BFS support ECMAScript's alternative operation.
+	      _GLIBCXX_DEBUG_ASSERT(__dfs_mode);
 	      _M_dfs(__match_mode, __state._M_alt);
 	      // Pick lhs if it matches. Only try rhs if it doesn't.
 	      if (!_M_has_sol)
diff --git a/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/ecma/wchar_t/63199.cc b/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/ecma/wchar_t/63199.cc
new file mode 100644
index 0000000..cbb23f7
--- /dev/null
+++ b/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/ecma/wchar_t/63199.cc
@@ -0,0 +1,69 @@
+// { dg-options "-std=gnu++11" }
+
+//
+// Copyright (C) 2014 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/>.
+
+#include <regex>
+#include <testsuite_hooks.h>
+#include <testsuite_regex.h>
+
+using namespace __gnu_test;
+using namespace std;
+
+// libstdc++/63199
+void
+test01()
+{
+  bool test __attribute__((unused)) = true;
+
+  std::setlocale(LC_ALL, "");
+
+  std::wstring current_token(L"II.");
+
+  std::vector<std::wregex> regex_vector;
+
+  for (int i = 0; i < 4; ++i)
+  {
+    std::regex_constants::syntax_option_type flag;
+    flag = std::regex_constants::ECMAScript | std::regex_constants::icase;
+
+    std::wregex reg;
+    reg.imbue(std::locale(""));
+    reg.assign(L"^(M*(?:CM|DC{1,3}|D|CD|C{1,3}){0,1}(?:XC|LX{1,3}|L|XL|X{1,3}){0,1}(?:IX|VI{0,3}|IV|I{1,3}){0,1}\\.)$", flag);
+
+    regex_vector.emplace_back(reg);
+  }
+
+  for (auto cit = regex_vector.cbegin(); cit != regex_vector.cend(); ++cit)
+  {
+    std::wstring::const_iterator it1 = current_token.begin();
+    std::wstring::const_iterator it2 = current_token.end();
+    std::wsmatch current_token_match;
+
+    regex_match_debug(it1, it2, current_token_match, *cit);
+    VERIFY(current_token_match[0] == current_token);
+    VERIFY(current_token_match[1] == current_token);
+  }
+}
+
+int
+main()
+{
+  test01();
+  return 0;
+}


More information about the Libstdc++ mailing list