atomic_domain implementation is DefaultConstructible, which contradicts spec

In UPC++ spec draft 6 (2018.3) atomic_domain<T> was specified with type traits:

DefaultConstructible, MoveConstructible, MoveAssignable, Destructible

Starting in UPC++ spec draft 8 (2018.9), the MoveAssignable and DefaultConstructible traits (and associated function templates) were removed from the spec.

The atomic_domain move assignment operator was removed from the implementation in 89ade1d5 (release 2018.9.0), but the default constructor remains in the implementation today (up to and including release 2019.9.0), which means the atomic_domain implementation remains DefaultConstructible in practice. The behavior is to default construct an object in the same state as a destroy()ed atomic_domain, which prohibits any operation other than the C++ destructor.

The spec change was made in spec commit 1d84eb9 which was part of this side PR. This change was discussed/resolved in our 2018-09-12 developer meeting and recorded as:

AI AMIR: remove MoveAssignable and DefaultConstructible from AD, at least for now

Presumably this was done for uniformity with the other collective objects team and dist_object (which match the spec in this regard), but I cannot find any other record for our specific rationale in applying this change to atomic_domain.

Lacking a move assignment operator, the only place I can think of where users might reasonably and usefully rely upon this property is in a class that conditionally uses atomics, eg:

struct foo {
  upcxx::atomic_domain<int32_t> ad;
  bool using_atomics;
  foo() : using_atomics(false) {
    cout << "This foo DOES NOT use atomics and has no restrictions on construction/destruction" << endl; 
  }
  foo(int x) : ad({atomic_op::load, atomic_op::store}), using_atomics(true) {
     cout << "This foo DOES use atomics, and must be constructed/destructed collectively on the master persona" << endl;
  }
  ~foo() {
    cout << "Destroying a foo. using_atomics=" << using_atomics << endl;
    if (using_atomics) ad.destroy();
  }
};

This compliance divergence unfortunately means that current user code might be relying on default construction of atomic_domain<T>, even though we do not guarantee that property. Preserving the current behavior that permits default construction is not at all problematic for the implementation, because this state already needs to exist to correctly support destroy(). However the continuing divergence from the spec is slightly concerning and the longer it persists the harder it could be for users to remove this usage later.

I'd like to see us move towards resolving this compliance violation in one of the following ways:

Restore the DefaultConstructible trait and constructor in the specification, or
Remove the default constructor from the atomic_domain implementation and document this as a breaking change in this release, or
Document the atomic_domain default constructor as deprecated in the release notes, decorate the implementation with [[deprecated]] or __attribute__ ((deprecated)) where available, and remove it in a subsequent release with a breaking change notice.

I don't currently have a strong preference.

Thoughts? @Amir Kamil @john bachan @Rob Egan @Steven Hofmeyr

Comments (2)