NVIDIA / stdexec

`std::execution`, the proposed C++ framework for asynchronous and parallel programming.
Apache License 2.0
1.56k stars 159 forks source link

questions on reference implementation support #499

Open huixie90 opened 2 years ago

huixie90 commented 2 years ago

Hi,

This is a question rather than an issue. I could not find a better place to ask this question. I apologies if this is not the way to ask question.

I am evaluating sender/receiver with our code base. The best implementation I could find is this reference implementation (libunifex is not an option due to its "experimental" natural). I have several questions.

  1. It doesn't seem to support MSVC. I tried to build the code with latest MSVC and immediately hit 6-7 issues because of different compiler bugs and name clashes with msvc stl internals. Is supporting MSVC on the plan? If so, I am happy to submit PRs that work arounds msvc bugs.

  2. Is it going to support C++20 for the long term (until paper accepted and all stl implemented them)? For example, if we are going to use it but this implementation switched to use some C++23 and C++26 features, then it will break our code. I guess this is likely to happen as p2547r0 is meant to replace the tag_invoke

Thanks Hui

ericniebler commented 2 years ago

libunifex is not an option due to its "experimental" natural

libunifex is far more baked than this reference implementation. However, it's interface has not kept pace with P2300, and bringing it in line with the current spec would be a huge job.

It doesn't seem to support MSVC.

We haven't been testing with msvc. The goal of this project is to be a clean reference implementation, largely for the purposes of standardization. I don't (yet) plan to support this for production use, and I would rather not obfuscate the code with compiler work-arounds, which would be contrary to its purpose.

Is it going to support C++20 for the long term

This library will change, often in backward-incompatible ways, as standardization progresses. Since P2300 is now targeting C++26, I won't commit to supporting C++20 indefinitely.

All that said, I'm very interested in fixing bugs in the code, so if you find bugs, please file them here. I'm also interested in the name clashes with the msvc stdlib. I would gladly accept a patch addressing that.

And finally, if you find a way to avoid msvc's bugs without making the code worse ("worse" of course being subjective), I'll take those patches as well. But I don't intend to slow development to support msvc. It would be on others such as yourself to keep it working.

My suggestion would be to fork the reference implementation, upstream what changes you can, and keep the more invasive changes local to your fork.

HTH

huixie90 commented 2 years ago

@ericniebler Thanks very much for your reply.

libunifex is far more baked than this reference implementation Unfortunately the repo explicitly says it is "experimental" which makes it really hard for us to approve to use.

if you find a way to avoid msvc's bugs without making the code worse Here are several things I tried to make hello_world.cpp to compile with msvc (not there yet) https://github.com/brycelelbach/wg21_p2300_std_execution/compare/main...huixie90:msvc

to be more specific, here are some msvc issues:

  1. __await is a keyword in msvc. some variable names are called __await which clashes with msvc https://github.com/brycelelbach/wg21_p2300_std_execution/commit/179f9131bdb90f576e00cc3dca810a6362c8252c

  2. duplicated typedef causing msvc to error https://github.com/brycelelbach/wg21_p2300_std_execution/commit/3941578e1fa4433f71acea36848fccc341dcd8ba

  3. MSVC is confused with this fold expression, which can be fixed with a local consteval helper function (the commit below has a bug where it should be disjunction instead of conjunction) https://github.com/brycelelbach/wg21_p2300_std_execution/commit/250fcb41e51d6b23256f886b18640bf9ea3f9fa9

  4. somehow msvc isn't happy with the variable name __valid after including <type_traits> https://github.com/brycelelbach/wg21_p2300_std_execution/commit/7654e6d535c69f98953b0539f55e51c7b2929c70

  5. compiler warning narrow conversion from size_t to bool https://github.com/brycelelbach/wg21_p2300_std_execution/commit/f002573de223932e34ae978235139b6938ab128c

  6. compiler warning. local variable hides global variable when_all https://github.com/brycelelbach/wg21_p2300_std_execution/commit/0ebe3b54b98f598341bcb20dc2ce6e8ae10596c9

  7. currently working on a tricky one https://godbolt.org/z/dEvoshMxa This is a simplified code that is related to when_all's implementation of operation_state where it has a tuple of the underlying operation_states which are non-movable.

My suggestion would be to fork the reference implementation, upstream what changes you can, and keep the more invasive changes local to your fork.

This sounds a doable solution.

Thanks Hui

Vigilans commented 2 years ago

@ericniebler I am also wondering what should a library developer (that will reference this repository for std::execution support) adopt in their own library that provides customization points, before the potential official language support in c++26...: