[libcxx-commits] [PATCH] D68480: Implementation of C++20's P1135R6 for libcxx

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

Please see the `Adding a new header` section of `NOTE.txt` (https://github.com/llvm-mirror/libcxx/blob/master/NOTES.TXT#L19-L29)



================
Comment at: include/__threading_support:429
+
+bool __libcpp_semaphore_wait_timed(__libcpp_semaphore_t* __sem, chrono::nanoseconds const& __ns) 
+{
----------------
Pass `nanoseconds` by value.


================
Comment at: include/atomic:1557
+    if(0 != __cxx_atomic_exchange(__cxx_atomic_rebind(&c->credit), (ptrdiff_t)0, memory_order_relaxed)) {
+        __libcpp_mutex_lock(&c->mutex);
+        __libcpp_mutex_unlock(&c->mutex);
----------------
What's the purpose of the lock and unlock?


================
Comment at: include/barrier:80
+    atomic<ptrdiff_t>  expected_adjustment;
+    CompletionF        completion;
+
----------------
Is the completion function ever stateless? If so we'll want to find a way to EBO it.


================
Comment at: include/barrier:91
+    };
+    ::std::vector<__state_t>   state;
+
----------------
EricWF wrote:
> `vector` should resolve to the correct name. No need to qualify it.
Also, I think `vector` can be replaced with a smaller dependency, like `unique_ptr<T[]>`. 


================
Comment at: include/barrier:147
+    __barrier_base(ptrdiff_t expected, CompletionF completion = CompletionF()) 
+            : expected(expected), expected_adjustment(0), completion(completion), 
+              phase(0), state((expected+1) >> 1)
----------------
`std::move(completion)`.


================
Comment at: include/barrier:176
+    {
+        __libcpp_thread_poll_with_backoff([&]() -> bool {
+            return phase.load(memory_order_acquire) != old_phase;
----------------
by-value capture seems like the safer choice here.


================
Comment at: include/barrier:198
+
+    alignas(64) atomic<ptrdiff_t> expected, arrived;
+    alignas(64) CompletionF       completion;
----------------
One line, one declaration please.


================
Comment at: include/barrier:199
+    alignas(64) atomic<ptrdiff_t> expected, arrived;
+    alignas(64) CompletionF       completion;
+    alignas(64) atomic<bool>      phase;
----------------
Why is this getting aligned?


================
Comment at: include/barrier:216
+    [[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+    arrival_token arrive(ptrdiff_t update = 1) 
+    {
----------------
Public declarations and member functions should be implemented in the class that declares them in the standard. 

Base classes containing implementation details should be inherited privately. 



================
Comment at: include/barrier:259
+
+    static inline _LIBCPP_INLINE_VISIBILITY
+    constexpr uint64_t __init(ptrdiff_t count) _NOEXCEPT 
----------------
Adding `inline` to implicitly inline function definitions has no effect except to act as an inline hint for the optimizer; and that's normally not our intention.


================
Comment at: include/barrier:262
+    {
+        return (((1u << 31) - count) << 32) 
+              | ((1u << 31) - count);
----------------
I'm pretty sure this needs an explicit cast to the 64 bit integer space.


================
Comment at: include/barrier:270
+    _LIBCPP_INLINE_VISIBILITY
+    __barrier_base(ptrdiff_t count, EmptyCompletionF = EmptyCompletionF()) 
+        : phase_arrived_expected(__init(count)) { 
----------------
Missing `explicit`?


================
Comment at: include/barrier:293
+    {
+        __libcpp_thread_poll_with_backoff([&]() -> bool
+        {
----------------
by-value capture seems like the safer choice here.


================
Comment at: include/chrono:1065
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
-#ifndef _LIBCPP_CXX03_LANG
+#if !defined(_LIBCPP_CXX03_LANG)
         duration() = default;
----------------
Is this change intentional?


================
Comment at: include/chrono:1571
 
+#ifndef _LIBCPP_HAS_CLOCK_API_EXTERNAL
+
----------------
Are these changes intentional?


================
Comment at: include/latch:103
+
+using latch = __latch_base;
+
----------------
The standard requires the declaration `class latch;`. So we can't use an alias here.


================
Comment at: src/atomic.cpp:9
+
+#ifndef _LIBCPP_HAS_NO_THREADS
+#include "atomic"
----------------
You should include `__config` to ensure you see this macro definition.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480