[libcxx-commits] [PATCH] D91992: [libc++] Add an extension to allow constructing std::thread from native handles

[JF Bastien via Phabricator via libcxx-commits]

jfb added a comment.

This is a sensible thing to support IMO. It's indeed weird to have native handle in other places, but not here.

What are the requirements on lifetime with this change? i.e. usually the thread owns the handle, but now it doesn't. Don't you need a bit to track "I don't own this" in the implementation? In particular, should the destructor act differently? Or is `thread` taking ownership at this point?

Check with @jwakely too, it would be better if libstdc++ had the same extension.

In D91992#2412467 <https://reviews.llvm.org/D91992#2412467>, @ldionne wrote:

> In D91992#2412458 <https://reviews.llvm.org/D91992#2412458>, @jfb wrote:
>
>> This is a sensible thing to support IMO. It's indeed weird to have native handle in other places, but not here.
>>
>> What are the requirements on lifetime with this change? i.e. usually the thread owns the handle, but now it doesn't. Don't you need a bit to track "I don't own this" in the implementation? In particular, should the destructor act differently? Or is `thread` taking ownership at this point?
>
> Conceptually, the `std::thread` owns the native handle after being constructed from one. So it would be more correct to use `thread(native_handle_type&&)`, and then you'd construct a `std::thread` with `std::thread t(std::move(handle));`. However, since native handles are basically C structs or even plain integers, IDK whether it makes sense to do that. Maybe?

Yeah that makes sense, but it's a bit surprising. More of a library design question at that point: do you want to have a `move_from` or `take` or whatever `static` member instead? So it's clear. I don't have a strong preference, it's just slightly surprising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91992