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

23 ↗(On Diff #521583)

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

29 ↗(On Diff #521583)

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

31 ↗(On Diff #521583)

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.


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.