Re: [PATCH] PR libstdc++/79162 ambiguity in string assignment due to string_view overload (LWG 2946)

[Jonathan Wakely <jwakely at redhat dot com>]

On 20/09/17 16:36 +0100, Jonathan Wakely wrote:
> On 04/09/17 16:48 +0100, Jonathan Wakely wrote:
> > On 30/07/17 15:01 +0200, Daniel Krügler wrote:
> > > 2017-07-28 22:40 GMT+02:00 Daniel Krügler <daniel.kruegler@gmail.com>:
> > > > 2017-07-28 22:29 GMT+02:00 Daniel Krügler <daniel.kruegler@gmail.com>:
> > > > > 2017-07-28 22:25 GMT+02:00 Tim Song <t.canens.cpp@gmail.com>:
> > > > > > On Fri, Jul 28, 2017 at 4:10 PM, Daniel Krügler
> > > > > > <daniel.kruegler@gmail.com> wrote:
> > > > > > > +      // Performs an implicit conversion from _Tp to __sv_type.
> > > > > > > +      template<typename _Tp>
> > > > > > > +        static __sv_type _S_to_string_view(const _Tp& __svt)
> > > > > > > +        {
> > > > > > > +          return __svt;
> > > > > > > +        }
> > > > > >
> > > > > > I might have gone for
> > > > > >
> > > > > > +        static __sv_type _S_to_string_view(__sv_type __svt) noexcept
> > > > > > +        {
> > > > > > +          return __svt;
> > > > > > +        }
> > > > > >
> > > > > > With that, we can also use noexcept(_S_to_string_view(__t)) to make up
> > > > > > for the absence of is_nothrow_convertible (basically the same thing I
> > > > > > did in LWG 2993's PR).
> > > > >
> > > > > Agreed, that makes very much sense. I will adjust the P/R, but before
> > > > > I resubmit I would like to get feedback whether the other two compare
> > > > > functions also should become conditionally noexcept.
> > > >
> > > > Locally I have now performed the sole change of the _S_to_string_view
> > > > declaration getting rid of the template, but would also like to gather
> > > > feedback from the maintainers whether I should also change the form of
> > > > the conditional noexcept to use the expression
> > > >
> > > > noexcept(_S_to_string_view(__t))
> > > >
> > > > instead of the current
> > > >
> > > > is_same<_Tp, __sv_type>::value
> > > >
> > > > as suggested by Tim Song.
> > > >
> > > > I'm asking also, because I have a paper proposing to standardize
> > > > is_nothrow_convertible submitted for the upcoming C++ mailing - This
> > > > would be one of the first applications in the library ;-)
> > >
> > > A slightly revised patch update: It replaces the _S_to_string_view
> > > template by a simpler _S_to_string_view function as of Tim Song's
> > > suggestion, but still uses the simplified noexcept specification
> > > deferring it to a future application case for is_nothrow_convertible.
> > > Furthermore now all three compare function templates are now
> > > (conditionally) noexcept by an (off-list) suggestion from Jonathan
> > > Wakely.
> >
> > I've committed this, after some whitespace fixes and testing.
> >
> > Thanks!
>
> This change causes two regressions in C++17 mode, see
> https://gcc.gnu.org/ml/gcc-testresults/2017-09/msg01674.html
>
> FAIL: 21_strings/basic_string/cons/char/moveable2.cc execution test
> FAIL: 21_strings/basic_string/cons/wchar_t/moveable2.cc execution test
>
> Here's a reduced version of that test, which passes in C++14 and fails
> in C++17:
>
> #include <string>
> #include <assert.h>
>
> class tstring : public std::string
> {
> public:
> tstring() : std::string() {}
> tstring(tstring&& s) : std::string(std::move(s)) {}
> };
>
> int main()
> {
> tstring b;
> b.push_back('1');
> tstring c(std::move(b));
> assert( c.size() == 1 && c[0] == '1' );
> assert( b.size() == 0 );
> }
>
> The second assertion fails, because this mem-initializer:
>
> tstring(tstring&& s) : std::string(std::move(s)) {}
>
> now prefers to use the new constructor:
>
> basic_string(const _Tp& __t, const _Alloc& __a = _Alloc())
>
> because tstring is convertible to string_view.
>
> This turns a non-allocating move into an allocating copy.

This patch fixes the failure above, I'm testing it now.

--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -115,6 +115,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
      template<typename _Tp, typename _Res>
       using _If_sv = enable_if_t<
         __and_<is_convertible<const _Tp&, __sv_type>,
+                __not_<is_convertible<const _Tp*, const basic_string*>>,
                __not_<is_convertible<const _Tp&, const _CharT*>>>::value,
         _Res>;