Skip to content

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Aug 16, 2021

Thanks @frederick-vs-ja for finding the issue at #2111

We didn't have move constructor and after adding range constructors (6fe02ac) string_view was no longer trivially move constructible.

reduced example(which I wanted to send to FE team (because @frederick-vs-ja thought it was a compiler issue)): https://gcc.godbolt.org/z/qqWbfce6s

Fixes DevDiv-1510475/VSO-1374748/AB#1374748.

@fsb4000 fsb4000 requested a review from a team as a code owner August 16, 2021 04:09
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Aug 16, 2021

Oh... You're right.
Perhaps what we need is an additional constraint (!same_as<remove_cvref_t<_Range>, basic_string_view>). I've submitted an LWG issue.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Aug 16, 2021

@frederick-vs-ja
Yes, you are right. I should change range constructor.
I opened: https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view and I didn't find

constexpr basic_string_view(basic_string_view&&) noexcept = default;

constructor.

Co-authored-by: Adam Bucior <[email protected]>
@jwakely
Copy link

jwakely commented Aug 16, 2021

Perhaps what we need is an additional constraint (!same_as<remove_cvref_t<_Range>, basic_string_view>).

FWIW libstdc++ already has that constraint.

@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 16, 2021
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Aug 17, 2021

I suspect their omission is the simplest way to specify that moves leave the source object unmodified.

@CaseyCarter I think this problem can resolved by WG21-P2251.

By the way, there're several types in the standard library whose move functions are suppressed by explicitly defaulted copy functions. The types listed here may be incomplete, some of which are able to propagate non-trivial copy operations:

  • std::span
  • std::basic_string_view
  • std::allocator
  • std::istreambuf_iterator
  • std::ostream_iterator
  • std::complex (specializations for float, double, and long double)
  • std::chrono::leap_second
  • std::istream_iterator
  • std::chrono::duration

I don't know whether it's intended to suppress the implicitly declared move functions. IMO it seems better to leave copy and move special member functions implicitly declared in the specification. (std::pmr::polymorphic_allocator, std::pmr::memory_resource, std::nested_exception, and std::ios_base::Init are likely in different situations.)

@jwakely
Copy link

jwakely commented Aug 17, 2021

IMO it seems better to leave copy and move special member functions implicitly declared in the specification.

I disagree. For a type where moving is identical to a copy I see no benefit to declaring a move constructor or move assignment operator. A separate move constructor makes sense if moving can do something different from a copy, define what moving means. If construction from an rvalue just does a bitwise copy, let that be done by the copy constructor.

The language does the right thing here: suppressing the declaration of the move constructor is exactly what we want.

@CaseyCarter
Copy link
Contributor

IMO it seems better to leave copy and move special member functions implicitly declared in the specification.

I disagree. For a type where moving is identical to a copy I see no benefit to declaring a move constructor or move assignment operator. A separate move constructor makes sense if moving can do something different from a copy, define what moving means. If construction from an rvalue just does a bitwise copy, let that be done by the copy constructor.

I do see a benefit in not declaring any SMFs when the implicitly-defined defaults suffice. The most readable spec is the spec you don't have to read at all. I don't think it's worth spending committee time to change the existing types, but if we were standardizing basic_string_view today on a green field I'd champion removing the SMF declarations.

@CaseyCarter
Copy link
Contributor

Looks great @fsb4000, thanks for fixing my bug in the implementation. Also thanks to @frederick-vs-ja for submitting the LWG issue to fix the corresponding defect I introduced in the Standard. (Yes, I make so many mistakes I need a posse of contributors to follow me around and fix them.)

@StephanTLavavej StephanTLavavej added performance Must go faster and removed bug Something isn't working labels Aug 18, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 18, 2021
@StephanTLavavej
Copy link
Member

FYI @CaseyCarter, @fsb4000 pushed small changes to address my code review comments after you approved.

@StephanTLavavej StephanTLavavej removed their assignment Aug 19, 2021
@CaseyCarter CaseyCarter removed the performance Must go faster label Aug 24, 2021
@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 24, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit c2e3f09 into microsoft:main Aug 27, 2021
@StephanTLavavej
Copy link
Member

Thanks again! 😻 I'll concede to @CaseyCarter in our label tug-of-war and will changelog this as a bugfix.

@CaseyCarter
Copy link
Contributor

Thanks again! 😻 I'll concede to @CaseyCarter in our label tug-of-war and will changelog this as a bugfix.

I'll admit that "We provide a non-standard guarantee that basic_string_view is trivially copyable" falls just short of implying that any particular operation is trivial, but I think every non-language-lawyer expects that copies and moves of a trivially copyable type are either invalid or trivial. That's really what most folk who have never attended WG21's Core Working Group think "trivially copyable" means =) So while the corrected behavior didn't break our stated contract it did break our implied contract.

@fsb4000 fsb4000 deleted the fix2111 branch August 27, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants