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

[Tim Shen <timshen at google dot com>]

On Mon, Sep 15, 2014 at 3:10 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> With this patch basic_regex::_M_traits becames a pointer, but that
> pointer becomes null after a move - is that safe?
>
> It looks wrong to allocate a traits object in the default constructor
> but not in the move constructor, and to allow _M_traits to be null
> following assign(), e.g. does this work OK after your patch?
>
>  std::regex r;
>  std::regex rr = std::move(r);
>  r.assign("a");
>  regex_match("a", r);

No it doesn't, sorry. The reason _M_traits can't be nullptr is imbue()
and getloc(), so should we implement move constructor using default
construction and swap?

> Would it make sense to store the traits object inside the NFA, instead
> of having it separate but tightly coupled to the NFA by references?

I've tried to do so before; it's better of course, but we still need
either _M_automaton or _M_automaton._M_traits to be heap allocated.
More over, it has to be non-null because of imbue() and getloc(), no
matter where it resides.

What do you think is the best practice here?

> I think for the 4.9 branch we could fix the copy constructor so it
> doesn't cause sharing of the NFA and then change the move constructor
> to also perform a copy, like so:
>
> diff --git a/libstdc++-v3/include/bits/regex.h
> b/libstdc++-v3/include/bits/regex.h
> index 9dc83fd..5a9dd78 100644
> --- a/libstdc++-v3/include/bits/regex.h
> +++ b/libstdc++-v3/include/bits/regex.h
> @@ -474,16 +474,17 @@ _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)
> +      : basic_regex(__rhs)
>       { }
>
>       /**
>
> This will be slower, and not noexcept, but avoids the dangling
> references.

That works but too bad for efficiency :( sorry again :(


-- 
Regards,
Tim Shen