Skip to content
This repository was archived by the owner on Mar 21, 2024. It is now read-only.

Tweak atomic and atomic_ref to match specification's constructor requirements #219

Merged
merged 2 commits into from
Nov 3, 2021

Conversation

wmaxey
Copy link
Member

@wmaxey wmaxey commented Oct 28, 2021

This is a bugfix to atomic as the previous refactor removed the deleted constructor and instead allows some internal machinery to create a default copy ctor for cuda::std::atomic.

This also adds a test to enforce this conformance.

@wmaxey wmaxey requested a review from griwes October 28, 2021 08:27
@wmaxey wmaxey force-pushed the bugfix/atomic_copy_ctor branch from 68e199b to 89847ef Compare October 28, 2021 08:28
@@ -1245,6 +1245,7 @@ struct __atomic_base {
mutable __cxx_atomic_impl<_Tp, _Sco> __a_;

__atomic_base() = default;
__atomic_base(const __atomic_base&) = delete;
Copy link
Collaborator

@jrhemstad jrhemstad Oct 28, 2021

Choose a reason for hiding this comment

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

What about copy assignment? We should probably just rule of 0 it here and be explicit about the move ctor/assignment too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, seems like we've also lost that at some point (probably also in the refactor?).

@wmaxey seems like upstream deletes assignments directly in atomic and we don't, but I guess doing it in the base class is fine too?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought process was: __atomic_base implements the behavior that atomic should inherit. This way if we 'rule of zero' atomic we should get the correct behavior.

We just don't have tests enforcing that bit of the standard so it was missed. 👎

@wmaxey wmaxey requested review from jrhemstad and griwes October 28, 2021 22:08
@wmaxey
Copy link
Member Author

wmaxey commented Oct 28, 2021

I've updated this to refine the constructors for move, and I'm intentionally ignoring volatile for now. :)

@wmaxey wmaxey force-pushed the bugfix/atomic_copy_ctor branch from 7cf86dc to 8169019 Compare October 28, 2021 22:12
@wmaxey wmaxey added the testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). label Nov 3, 2021
@wmaxey wmaxey merged commit ecab64a into main Nov 3, 2021
@wmaxey wmaxey deleted the bugfix/atomic_copy_ctor branch November 3, 2021 19:42
@wmaxey wmaxey added testing: internal ci passed Passed internal NVIDIA CI (DVS). and removed testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants