[libcxx-commits] [PATCH] D145183: [libc++][In Progress] Implement `stop_token`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I mainly glanced over the patch and didn't look at the wording.



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:107
 "`P0645R10 <https://wg21.link/P0645R10>`__","LWG","Text Formatting","Cologne","|Complete| [#note-P0645]_","14.0"
-"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","",""
+"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","In Progress",""
 "`P0784R7 <https://wg21.link/P0784R7>`__","CWG","More constexpr containers","Cologne","|Complete|","12.0"
----------------
After this patch it's partial :-) Can you document which parts are still missing?

We have multiple partial done papers, without any information what is and what is not done. That makes it hard to finish these papers.


================
Comment at: libcxx/include/__stop_token/stop_source.h:33
+public:
+  stop_source() : __ref_counted_state_(__ref_counted_stop_state::__new_state_from_stop_source_tag{}) {}
+
----------------
I miss `_LIBCPP_HIDE_FROM_ABI` on most (all?) functions.


================
Comment at: libcxx/include/__stop_token/stop_state.h:47
+
+  //       31 - 2          |  1     |    0           |
+  //  stop_source counter  | locked | stop_requested |
----------------
maybe move constants `__stop_requested_bit` here too, that makes it clear they belong together.


================
Comment at: libcxx/include/__stop_token/stop_state.h:190
+  void __remove_callback(__stop_callback_base* __cb) noexcept {
+    __lock();
+
----------------
Did you consider using RAII based locking?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183