DaanDeMeyer / reproc

A cross-platform (C99/C++11) process library
MIT License
552 stars 65 forks source link

Move to C++17 and use std::optional instead of reference in reproc::event::source. #40

Closed DaanDeMeyer closed 1 year ago

DaanDeMeyer commented 4 years ago

Current reference based design was a bad decision as it forces a process to be assigned to an event source whereas in reproc the process is optional. This creates a significant mismatch between the two APIs.

The big problem is that the idiomatic solution to this problem requires std::optional which is only available from C++17 onwards.

I tried moving the reproc++ polling interface into a separate library reproc++-poll that could be C++17 only without requiring C++17 for the rest of the reproc++ interface and while this works well, there's one massive blocker, reproc::drain and subsequently reproc::run as well are built on reproc::poll so those would have to move to reproc++-poll as well which would remove most of the useful stuff from reproc++.

We could use a pointer instead of a reference as an interim fix but this clashes with the value based design of reproc::process.

So the best solution seems to be to wait until we can move reproc++ to C++17. When we do this depends on reproc++'s users. I have no data on this, but I'm guessing the poll interface isn't actually used that much and for now, making sure reproc++ works with C++11 is probably more important for most users than having a better interface for reproc++::poll.

I'm opening this issue for visibility on the issue and to allow anyone to give feedback on whether reproc++ should move to C++17 to fix this issue or stay on C++11 to be a viable option for a larger range of projects. Eventually the move will happen, it's just a matter of deciding when exactly. My current intent is to not do the move unless there's a pressing need for a better reproc::poll API or some data shows up that indicates a majority of C++ developers are using at least C++17.

redtide commented 3 years ago

what about optional-lite?

DaanDeMeyer commented 3 years ago

I'd prefer to keep reproc with zero dependencies but yeah, a backfill of std::optional would also work.

DaanDeMeyer commented 1 year ago

Turns out we don't really need optional, we just need to have the event source own the process