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

[Jonathan Wakely <jwakely at redhat dot com>]

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()));