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

[Jonathan Wakely]

On 29/09/14 10:43 -0700, Tim Shen wrote:
>On Tue, Sep 16, 2014 at 1:39 PM, Tim Shen <timshen@google.com> wrote:
>> On Tue, Sep 16, 2014 at 2:23 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> You also mentioned not storing any references to the traits in the
>>> automaton and passing it in when it executes, how difficult would that
>>> be?
>>
>> Nah, that's too bad; it makes move/copy non trivial, not even O(1).
>
>It's a wrong answer because I misread the question. Doing that
>requires non-trivial executor interfaces change, but move/copy will be
>still O(1). The regex match/search will be a little bit slowed down.
>Anyway, it's already fixed in trunk now in the ideal way, not this
>one.
>
>Here's the abi compatible patch for "gcc-4_9-branch". I'm so sad for
>the slow copy/move/assign/swap :(
>
>Bootstrapped and tested.
>
>Thanks!
>
>
>-- 
>Regards,
>Tim Shen

>commit 647e2e917e8571b60e49c4314c4dec708ecf497d
>Author: timshen <timshen@google.com>
>Date:   Sun Sep 28 19:35:51 2014 -0700
>
>    	* include/bits/regex.h (basic_regex::basic_regex, basic_regex::assign,
>    	basic_regex::swap): Fix dangling _M_traits reference problem.
>    	* testsuite/28_regex/algorithms/regex_match/ecma/wchar_t/63199.cc:
>    	New test case.

Please put the PR number in the ChangeLog, so it updates Bugzilla
automatically.

>diff --git a/libstdc++-v3/include/bits/regex.h b/libstdc++-v3/include/bits/regex.h
>index 9dc83fd..7e3ab6c 100644
>--- a/libstdc++-v3/include/bits/regex.h
>+++ b/libstdc++-v3/include/bits/regex.h
>@@ -473,17 +473,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        * @brief Copy-constructs a basic regular expression.
>        *
>        * @param __rhs A @p regex object.
>+       *
>+       * The implementation is a workaround concerning ABI ompatibility. See:

You've spelled it "ompatibility" everywhere. I think we only need the
note once, maybe on the move constructor or rvalue basic_regex::assign.

>+       * https://gcc.gnu.org/ml/libstdc++/2014-09/msg00067.html
>        */
>-      basic_regex(const basic_regex& __rhs) = default;
>+      basic_regex(const basic_regex& __rhs)
>+      : _M_flags(__rhs._M_flags), _M_original_str(__rhs._M_original_str)
>+      { this->imbue(__rhs.getloc()); }

This solves the problem quite nicely, given the constraints we have.

>       /**
>        * @brief Move-constructs a basic regular expression.
>        *
>        * @param __rhs A @p regex object.
>+       *
>+       * The implementation is a workaround concerning ABI ompatibility. See:
>+       * https://gcc.gnu.org/ml/libstdc++/2014-09/msg00067.html
>        */
>-      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)
>+      : basic_regex(__rhs)
>       { }

This just copies but we could still make it slightly more efficient by
moving _M_original_str (then resetting __rhs to an empty state).

      basic_regex(basic_regex&& __rhs)
      : _M_flags(__rhs._M_flags), _M_original_str(std::move(_M_original_str))
      {
        this->imbue(__rhs.getloc());
        __rhs._M_automaton.reset();
      }

This is only slightly more efficient, and still needs to allocate
memory for the NFA, so I'm not sure if it's worth it. Your call.

>@@ -600,14 +614,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        * @brief The move-assignment operator.
>        *
>        * @param __rhs Another regular expression object.
>+       *
>+       * The implementation is a workaround concerning ABI ompatibility. See:
>+       * https://gcc.gnu.org/ml/libstdc++/2014-09/msg00067.html
>        */
>       basic_regex&
>-      assign(basic_regex&& __rhs) noexcept
>-      {
>-	basic_regex __tmp(std::move(__rhs));
>-	this->swap(__tmp);
>-	return *this;
>-      }
>+      assign(basic_regex&& __rhs)
>+      { return this->assign(__rhs); }

This could std::move(__rhs._M_original_str) rather than just copy, as
suggested above for the move constructor.

>       void
>       swap(basic_regex& __rhs)
>       {
> 	std::swap(_M_flags, __rhs._M_flags);
> 	std::swap(_M_traits, __rhs._M_traits);
>-	std::swap(_M_automaton, __rhs._M_automaton);
>+	std::swap(_M_original_str, __rhs._M_original_str);
>+	auto loc = __rhs.getloc();
>+	__rhs.imbue(this->getloc());
>+	this->imbue(loc);

Because imbue() returns the old locale you can swap them like this:

        imbue(__rhs.imbue(getloc()));