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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch! If you upload the patch with `arc`, it should also include some context in the diff, which is useful. Let us know if you run into issues with that.



================
Comment at: libcxx/include/latch:75
     }
 
----------------
Not attached: Can you please add some tests for these assertions? To see how we do this for other assertions: `libcxx/test/libcxx/containers/views/views.span/span.elem/assert.back.pass.cpp`.


================
Comment at: libcxx/include/latch:80-81
+    {
+        _LIBCPP_ASSERT_UNCATEGORIZED(__expected >= 0,
+                                     "__expected cannot be negative");
+    }
----------------
I think it is also UB if `__expected > max()`, right? If so, let's add an assertion.


================
Comment at: libcxx/include/latch:91-92
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update >= 0, "latch::count_down called with a negative value");
         auto const __old = __a_.fetch_sub(__update, memory_order_release);
----------------
I read:

> If n is greater than the value of the internal counter or is negative, the behavior is undefined.

So we can actually check more than just `__update >= 0`.


================
Comment at: libcxx/include/latch:112-113
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update >= 0, "latch::arrive_and_wait called with a negative value");
         count_down(__update);
----------------
Same here:

> If n is greater than the value of the internal counter or is negative, the behavior is undefined.



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

https://reviews.llvm.org/D154425