[libcxx-commits] [PATCH] D155399: [libc++] add basic runtime assertions to <semaphore>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/semaphore:102
+        if (0 < __old)
+        {
+            // Nothing to do
----------------
I suggest we move to `__old > 0` as a drive-by fix too, since it's more consistent with the rest of the code (and it's easier to read, although that is subjective).


================
Comment at: libcxx/test/libcxx/thread/thread.semaphore/assert.ctor.pass.cpp:11
+
+// REQUIRES: availability-synchronization_library-missing
+// XFAIL: target={{.+}}-apple-macosx10.{{13|15}}
----------------
This `REQUIRES` shouldn't be there, otherwise we're basically requiring an old system that didn't have the synchronization library. Along with the XFAIL below, this means that this test was never run.


================
Comment at: libcxx/test/libcxx/thread/thread.semaphore/assert.release.pass.cpp:11-12
+
+// REQUIRES: availability-synchronization_library-missing
+// XFAIL: target={{.+}}-apple-macosx10.{{13|15}}
+
----------------
Same, both these lines should be removed.


================
Comment at: libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp:33-36
+void not_positive() {
+  // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}The least maximum value must be a positive number}}
+  std::counting_semaphore<-1> s(2);
+  (void)s;
----------------
This test is a `.compile.pass.cpp`, so your `expected-error` comment is never picked up. You want to move this to a separate `.verify.cpp` test instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155399/new/

https://reviews.llvm.org/D155399