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

[Jonathan Wakely]

On 20/09/17 18:59 +0100, Jonathan Wakely wrote:
>On 20/09/17 17:52 +0100, Jonathan Wakely wrote:
>>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>;
>>
>>
>
>I'm committing this to fix the regression. If the final LWG 2946
>resolution does something different we can change it again.
>
>Tested powerpc64le-linux, committed to trunk.
>
>I'll start another test run with all variations ...

That test run revealed I forgot to make the same change to the _If_sv
helper in the old std::basic_string.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 981 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20170920/0e0102de/attachment.bin>