This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Utilities for implementing stop_token

Authored by huixie90 on May 9 2023, 8:51 AM.


Group Reviewers
Restricted Project
rG544e38ca439f: [libc++] Utilities for implementing stop_token

This change contains three util classes that were out from D145183 to make incremental progress

  • automic_unique_lock
  • intrusive_list
  • intrusive_shared_ptr

Diff Detail

Event Timeline

huixie90 created this revision.May 9 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 8:51 AM
huixie90 updated this revision to Diff 520788.May 9 2023, 12:15 PM

add tests

huixie90 published this revision for review.May 12 2023, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2023, 3:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 12 2023, 8:59 AM
ldionne added a subscriber: ldionne.

I really like this! Thanks a lot for splitting this into another review, I think this makes the stop_token code much easier to understand. LGTM with some comments.


Nit: let's use a c++ style comment


Note for yourself: you think it might be possible to instead own the elements of the list and simplify some stuff in stop_callback?


Maybe a short comment explaining what the class does? Mention that it doesn't own the nodes.


Currently, the list can be copied or copy-assigned. I think this is a bit misleading, since the list doesn't really own the elements. Normally, we expect that two copies of an object are independent, which is not the case here since it's not a value type. In a way, this is some kind of "view" like string_view. The "structure" created by the list elements exists regardless of whether we have an __intrusive_list object alive somewhere, and the __intrusive_list only gives us an API to manipulate it along with a handle to the first element in that "structure".

So I would suggest one of two things:

  1. Make it non-copyable, and use
__intrusive_list() = default;
__intrusive_list(__intrusive_list const&) = delete;
__intrusive_list(__intrusive_list&&) = default;
__intrusive_list& operator=(__intrusive_list const&) = delete;
__intrusive_list& operator=(__intrusive_list&&) = default;
  1. Change the name slightly to something like __intrusive_list_view (??) to make it clearer that it's a view. Along with a comment explaining the design of the class, I think this can go a long way. Then you probably also want to make the special members explicit like above with = default for everything, just to call it out. And finally, if you go for that approach, don't forget to update file names, header guards and the test!

Edit: Since the destructor doesn't destroy the elements in the list, I think we now agree that (2) is probably the way to go.

25 ↗(On Diff #521583)

Can you also test when the lock bit is not the first one? It should be easy to do something like:

template <uint8_t LockBit>
void test() {
  // all the stuff

int main(...) {
  // etc..
This revision is now accepted and ready to land.May 12 2023, 8:59 AM
huixie90 updated this revision to Diff 521730.May 12 2023, 11:19 AM
huixie90 marked 6 inline comments as done.

address comments

ldionne accepted this revision.May 15 2023, 11:39 AM

Thanks, nice!

This revision was automatically updated to reflect the committed changes.