Skip to content

Conversation

@StephanTLavavej
Copy link
Member

Fixes #1497. While atomic and atomic_ref are similar in many ways, they need different static_asserts:

✔️ atomic

WG21-N4868 31.8.1 [atomics.types.generic.general]/1: "The program is ill-formed if any of is_­trivially_­copyable_­v<T>, is_­copy_­constructible_­v<T>, is_­move_­constructible_­v<T>, is_­copy_­assignable_­v<T>, or is_­move_­assignable_­v<T> is false."

STL/stl/inc/atomic

Lines 2165 to 2168 in 19c683d

static_assert(is_trivially_copyable_v<_Ty> && is_copy_constructible_v<_Ty> && is_move_constructible_v<_Ty>
&& is_copy_assignable_v<_Ty> && is_move_assignable_v<_Ty>,
"atomic<T> requires T to be trivially copyable, copy constructible, move constructible, copy assignable, "
"and move assignable.");

🪲 atomic_ref

WG21-N4868 31.7.1 [atomics.ref.generic.general]/2: "The program is ill-formed if is_­trivially_­copyable_­v<T> is false."

STL/stl/inc/atomic

Lines 2350 to 2353 in 19c683d

static_assert(is_trivially_copyable_v<_Ty> && is_copy_constructible_v<_Ty> && is_move_constructible_v<_Ty>
&& is_copy_assignable_v<_Ty> && is_move_assignable_v<_Ty>,
"atomic_ref<T> requires T to be trivially copyable, copy constructible, move constructible, copy assignable, "
"and move assignable.");

Testing

I looked into testing a non-copy-constructible, yet trivially-copy-assignable, type. However, because atomic_ref::store passes T by value, it appears that this doesn't actually work, and it's unclear from the Standardese whether it's supposed to. So, this PR simply fixes the static_assert to match the Standardese and tests atomic_ref<const int>.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Nov 24, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 24, 2020 08:54
@miscco
Copy link
Contributor

miscco commented Nov 24, 2020

I believe non-copy-constructible yet copy-assignable types would not be valid. According to https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable a type is only trivially copyable iff:

  • At least one copy constructor, move constructor, copy assignment operator, or move assignment operator is eligible
  • Every eligible copy constructor (if any) is trivial
  • Every eligible move constructor (if any) is trivial
  • Every eligible copy assignment operator (if any) is trivial
  • Every eligible move assignment operator (if any) is trivial
  • Has a trivial non-deleted destructor

So a type that is not copy-constructible cannot be trivially copyable

Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and given the requirements on trivially-copyable I believe the changes are correct

@StephanTLavavej
Copy link
Member Author

Thanks for reviewing my code, I appreciate it! 😸

So a type that is not copy-constructible cannot be trivially copyable

Clang, GCC, and MSVC accept this: https://godbolt.org/z/W4zrnY

#include <type_traits>
using namespace std;

struct Integer {
    int n{0};
    Integer() = default;
    Integer(const Integer&) = delete;
    Integer& operator=(const Integer&) = default;
};

static_assert(is_trivially_copyable_v<Integer>);
static_assert(!is_copy_constructible_v<Integer>);
static_assert(is_copy_assignable_v<Integer>);
static_assert(is_trivially_copy_assignable_v<Integer>);

int main() {}

@miscco
Copy link
Contributor

miscco commented Nov 24, 2020

Ah, reading it again, it an exclusive list http://eel.is/c++draft/class.prop#1

@AlexGuteniev
Copy link
Contributor

I would not expect store to work even with copy constructible types. Guess you shouldn't be able to modify a constant accidentally.

@AlexGuteniev
Copy link
Contributor

Ah, you mean separate test for non-constant odd type? Ok, then it is really an interesting case

@StephanTLavavej StephanTLavavej merged commit 2dbcfa5 into microsoft:master Dec 3, 2020
@StephanTLavavej StephanTLavavej deleted the atomic_ref_static_assert branch December 3, 2020 23: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.

<atomic>: atomic_ref<const T> fails to compile

7 participants