[PATCH] D22557: [libcxx] Diagnose invalid memory order arguments in <atomic>. Fixes PR21179.

[JF Bastien via cfe-commits]

jfb added a subscriber: jfb.
jfb added a comment.

Awesome, thanks for doing this!

Should this be a warning or an error?


================
Comment at: include/atomic:581
@@ +580,3 @@
+                               || __f == memory_order_acq_rel, "")))          \
+    __attribute__ ((__unavailable__("memory order argument to atomic operation is invalid")))
+#endif
----------------
This isn't checking for the following requirement from the standard:

> The failure argument shall be no stronger than the success argument.

I think that's OK because I intend to remove that requirement from C++ :)

Should we nonetheless enforce the requirement until the standard drops it? If so, "stronger" isn't well defined by the standard, details here: https://github.com/jfbastien/papers/blob/master/source/D0418r1.bs

================
Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:30
@@ +29,3 @@
+        vx.load(std::memory_order_release); // expected-error {{operation is invalid}}
+        vx.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
----------------
Could you test that the other memory orders don't have any diagnostic (here and below).

================
Comment at: test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp:55
@@ +54,3 @@
+    }
+    // compare exchange weak
+    {
----------------
For cmpxchg (weak and strong), should you also test the version with only a `success` ordering? The `failure` one is auto-generated from `success`, but it would be good to know that it never generates a diagnostic.


https://reviews.llvm.org/D22557