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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne abandoned this revision.
ldionne added a comment.

In D91992#2412493 <https://reviews.llvm.org/D91992#2412493>, @__simt__ wrote:

> Ok, backing up: should libcxx have this extension?
>
> As I said, I'm not opposed to it but a little while ago I was told that libcxx doesn't do extensions in the mainline. I don't remember what it was I had in mind, maybe it was atomic<>::wait() support for timeouts. At the time, the answer was to do this in a deployment fork, not mainline.

You're right, actually I'm probably the one who said that. As we said offline, I think it might make it more reasonable to support an extension if there's a proposed paper for it, but this isn't the case here. It's fine to carry extensions downstream, and that's what I'll do. Thanks for reminding me :).

In D91992#2413447 <https://reviews.llvm.org/D91992#2413447>, @jwakely wrote:

> This reminds me of an often-requested extension to `std::fstream` to allow it to take ownership of a `FILE*` or unix file descriptor. In libstdc++ we do support that, but only by using a non-standard filebuf type, `__gnu_cxx::stdio_filebuf`. It's not available for `std::basic_filebuf`. Doing that would be an alternative design for this (that is, a non-standard `native_thread` type that you'd use instead of `std::thread`). That wouldn't be interchangeable with `std::thread` though, so I can see why you want to extend the existing type.
>
> I was going to mention that p2019 suggests a very different approach for platform-specific properties of the new thread, but I see you already covered that.
>
> I agree with Olivier that this would have to be conditionally supported, but that's already implied by the use of `native_handle_type` (which isn't required to exist). For libstdc++, we don't currently do anything special that's specific to C++ when creating a new thread, so we could support this exactly the same way.
>
> However, I'm concerned that this changes the result of things like `is_constructible<thread, int>` or `is_constructible<thread, void*>`. On GNU/Linux `thread::native_handle_type` is just `unsigned long`, on MinGW-w64 it's `uintptr_t`, so this would mean you can construct a `std::thread` with any integer. On other targets it's a `void*`. I'd be happier if the new constructor required a tag type to say "I know what I'm doing", e.g. `thread(from_native_t, native_handle_type)` so you'd say `std::thread t(std::from_native, thr);`. Or JF's suggestion of a static member function would also work: `auto t = std::thread::from_native(thr);`

Right, thanks for these thoughts -- I agree that adding a public constructor taking `int` is not ideal.

I'll close this since I don't think it's reasonable to add this extension until the Standard has it, or there's a paper, or at least something. Thanks for your comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91992