On Sun, 19 Aug 2012 10:24:08 +0200, Alexander Lamaison <awl03@doc.ic.ac.uk> wrote: Hi Alexander,
[...]Macros. Boost Libraries rarely have macros but when they do, it's almost never for platform specific behaviour. The whole point is that that gets hidden in the implementation. At first glance it seems to me that all the uses of macros show in the documentation (including the ASIO one) could be 'taken inside'.
if I look at the tutorial at http://www.highscore.de/boost/process0.5/boost_process/tutorial.html I see three scenarios where #ifdefs must be used: * Cleaning up resources * Asynchronous I/O * Asynchronously waiting for a program to exit I'll talk about cleaning up resources after the next paragraph (where you mentioned RAII). For asynchronous I/O Boost.Process relies on Boost.Asio. Boost.Asio provides two I/O objects which are unfortunately platform-specific. While unfortunate I'm not sure whether this is a Boost.Process problem? It's just that because of Boost.Process we realize that we have no platform-independent class in Boost.Asio for native handles? It would be nice if there was something which for example could be initialized with a boost::iostreams::file_descriptor_sink or boost::iostreams::file_descriptor_source. But I wonder now whether I shouldn't have mentioned asynchronous I/O in the Boost.Process documentation as this wouldn't be a Boost.Process problem then. ;) Asynchronously waiting for a program to exit is a similar problem but worse. While boost::asio::posix::stream_descriptor and boost::asio::windows::stream_handle are somewhat similar, waiting requires to use the very different I/O objects boost::asio::signal_set and boost::asio::windows::object_handle. The system APIs are unfortunately that different: While you need to catch a signal on POSIX you have to use a wait function on Windows. The challenge is to rewrite the code in the example at http://www.highscore.de/boost/process0.5/boost_process/tutorial.html#boost_p... in a platform-independent way. (Actually I tried this in Boost.Process 0.4. We had a class there called status which had an async_wait() function which worked on POSIX and Windows - if you were careful. The implementation was rather horrible and heavily criticized. Because of that I created boost::asio::windows::object_handle - that's the only reason why we can now actually use Boost.Asio to wait asynchronously on POSIX and Windows even though we need to use platform-specific I/O objects. But it was a step forward.)
Non-RAII. IMHO, modern C++ code should not expect the caller to manage the cleanup themselves. If `discard` should only be called once the (shared) child process is no longer needed then either don't share the child process between instances (i.e. they become non-copyable) or manage the `child` instances' resources in a shared manner e.g. a shared_ptr that does whatever cleanup `discard` previously did in its destructor.
The challenge is here to rewrite the example from http://www.highscore.de/boost/process0.5/boost_process/tutorial.html#boost_p... in a platform-independent way. In this example I ignore SIGCHLD as that's rather easy to do. But if you come up with a platform-independent solution you need of course also consider that someone might want to clean up by fetching the exit code from the child process. The whole issue with signals is complicated anyway as it's a global setting in an application. Libraries need to cooperate and shouldn't steal signals or overwrite signal handlers. This looks like a pretty tough job to create a RAII type which works on Windows and POSIX?
Environment. The child inherits the environment on one platform be default but not the other. This sounds like a big gotcha. The docs say "on Windows environment variables are inherited by default". Quite so. But there must be a way to stop them being inherited. So why not do that by default on Windows and only let the true Windows default behaviour happen when the inherit_env initialiser is passed? Or vice versa with a suppress_env initialiser?
I agree, we need more initializers here. Right now there is no initializer either to easily define a new set of environment variables. As of today you can only use inherit_env. I blame my lack of time for not having created more useful initializers yet. :)
Nevertheless, I'm please to see the improvements you've been making to the library. Keep up the good work.
Thanks! And also thanks for your feedback! Boris