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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

LGTM, though I spot 4 new checks and 3 new tests. Is there one missing?



================
Comment at: libcxx/include/semaphore:99
+        auto __old = __a_.fetch_add(__update, memory_order_release);
+        _LIBCPP_ASSERT_UNCATEGORIZED(__update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
+
----------------
Shouldn't this check be using `::max()` rather than the default `_LIBPCP_SEMAPHORE_MAX`?


================
Comment at: libcxx/include/semaphore:102
+        if (0 < __old)
             ;
+        else if (__update > 1)
----------------
Please put an empty `{ }` block here to make it more clear that this is intentional. Maybe a comment as well.


================
Comment at: libcxx/include/semaphore:141
 class counting_semaphore
 {
     __atomic_semaphore_base __semaphore_;
----------------
We should probably add a static assertion that `__least_max_value` is positive.


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

https://reviews.llvm.org/D155399