Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misplaced static_assert in basic_field's move-assignment operator #2517

Open
Tradias opened this issue Sep 8, 2022 · 7 comments
Open

Misplaced static_assert in basic_field's move-assignment operator #2517

Tradias opened this issue Sep 8, 2022 · 7 comments

Comments

@Tradias
Copy link

Tradias commented Sep 8, 2022

Version of Beast: 1.80.0

Steps necessary to reproduce the problem

https://godbolt.org/z/vdfYrEE1e

Error:

/opt/compiler-explorer/libs/boost_1_80_0/boost/beast/http/impl/fields.hpp: In instantiation of 'boost::beast::http::basic_fields<Allocator>& boost::beast::http::basic_fields<Allocator>::operator=(boost::beast::http::basic_fields<Allocator>&&) [with Allocator = std::pmr::polymorphic_allocator<>]':
<source>:7:31:   required from here
/opt/compiler-explorer/libs/boost_1_80_0/boost/beast/http/impl/fields.hpp:440:58: error: static assertion failed: Allocator must be noexcept assignable.
  440 |     static_assert(is_nothrow_move_assignable<Allocator>::value,
      |                                                          ^~~~~
/opt/compiler-explorer/libs/boost_1_80_0/boost/beast/http/impl/fields.hpp:440:58: note: 'boost::integral_constant<bool, false>::value' evaluates to false
Compiler returned: 1

Proposed solution

Move the static_assertion from fields.hpp#L440 to the noexcept specifier:

template<class Allocator>
auto
basic_fields<Allocator>::
operator=(basic_fields&& other) noexcept(
    std::conjunction_v<alloc_traits::propagate_on_container_move_assignment, is_nothrow_move_assignable<Allocator>>)
      -> basic_fields&
{

And actually move assign the allocator instead of copy assigning it in fields.hpp#L1132.

@madmongo1
Copy link
Collaborator

The same is true for std::vector.
https://godbolt.org/z/3rxj9sKs7

@vinniefalco What (if any) is the intention here?

@Tradias
Copy link
Author

Tradias commented Sep 8, 2022

@madmongo1 your example is malformed, here is the corrected one that compiles successfully: https://godbolt.org/z/aaKjbaoEc

@vinniefalco
Copy link
Member

@vinniefalco What (if any) is the intention here?

I have no idea to be honest. Allocators are cancer.

@xbreak
Copy link

xbreak commented Sep 23, 2022

I ran into this as well and found related background in p0337 - Delete operator= for polymorphic_allocator

Given that a polymorphic_allocator is not propagated on assignment, one may question the usefulness of the assignment operators. Indeed, it seems that most uses of assignment for polymorphic_allocator are likely to be errors.

@klemens-morgenstern
Copy link
Collaborator

The static assert doesn't seem to be misplaced, move-assignments work fine: https://godbolt.org/z/7Pnn99KzW

What is needed is support for propagate_on_container_move_assignment.

@klemens-morgenstern klemens-morgenstern added this to Needs triage in Taming the Beast Sep 24, 2022
@klemens-morgenstern klemens-morgenstern moved this from Needs triage to Planned in Taming the Beast Sep 24, 2022
@Tradias
Copy link
Author

Tradias commented Sep 24, 2022

@klemens-morgenstern ? Your example doesn't perform move-assignment at all: https://godbolt.org/z/vdfYrEE1e

@Tradias
Copy link
Author

Tradias commented Sep 24, 2022

Or maybe a more simplified example showing it is unrelated to propagate_on_container_move_assignment: https://godbolt.org/z/Kb8d66fb5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

5 participants