
AMDG Review part 2: implementation boost/process.hpp: boost/process/all.hpp: boost/process/child.hpp: The PP guard on the constructor that takes a handle, should be #if defined(BOOST_WINDOWS_API) || defined(BOOST_PROCESS_DOXYGEN) so that it appears in the reference. The map should be passed by const std::map<stream_id, handle>& (No need to make extraneous copies.) windows.h is not needed. boost/process/config.hpp: Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info? boost/process/context.hpp: boost/process/environment.hpp: boost/process/handle.hpp: The semantics of handle::dont_close appear to be the same as the original boost::iostreams::file_descriptor, in that an explicit call to close still closes the handle. This led to some problems, so Daniel James changed it to have three options. See ticket #3517. I'm wondering whether similar problems are liable to come up here. If the handle is supposed to be closed and construction fails, the native handle should be closed. Otherwise, it's difficult make code using it exception safe. boost/process/operations.hpp: find_executable_in_path: I'm not sure that asserting is the right response when the name contains a "/." It's not unreasonable to expect that the name can come from outside the program, and thus may not be valid. If you do go with assert, the precondition needs to be documented. On Windows, it's possible to have a path longer than MAX_PATH. You should probably use the size returned from SearchPathA to allocate a buffer that's big enough and try again. Cygwin executables end with .exe, just like windows. create_child: The context should be passed by const reference. The requirements on Context and Arguments need to be documented. Is not exception safe with BOOST_POSIX_API because if environment_to_envp throws, the memory allocated by collection_to_argv will not be released. I'd like everything after the child process is started to be no-throw, so I can guarantee that if the child is created, I can get a handle to it. This includes returning the child object. boost/process/pid_type.hpp: boost/process/pipe.hpp: Can you #include the appropriate specific header, rather than the whole boost/asio.hpp? boost/process/pistream.hpp: boost/process/postream.hpp: boost/process/process.hpp: boost/process/self.hpp: boost/process/status.hpp: boost/process/stream_behavior.hpp: boost/process/stream_ends.hpp: boost/process/stream_id.hpp: boost/process/stream_type.hpp: boost/process/detail/basic_status.hpp: Again, #include <boost/asio.hpp> is a lot more than you need. boost/process/detail/basic_status_service.hpp: The constructor is not exception safe. If starting the thread fails, the event will be leaked. work_thread can throw which will terminate the process. Is this the desired behavior or should it be using Boost.Exception to move the exception to a different thread? async_wait can leak the HANDLE created with OpenProcess. condition variables are allowed to have spurious wakeups. You need to track enough state to make sure that you don't wake up early. boost/process/detail/posix_helpers.hpp: environment_to_envp and environment_to_envp are not exception safe. boost/process/detail/status_impl.hpp: operation: If an argument is not used, virtual void operator()(int /* exit_code */) is the best way to suppress warnings. boost/process/detail/systembuf.hpp: I don't think that overflow is correct. According to the MSDN documentation WriteFile can return when: * The number of bytes requested is written. * A read operation releases buffer space on the read end of the pipe (if the write was blocked). For more information, see the Pipes section. * An asynchronous handle is being used and the write is occurring asynchronously. * An error occurs. Note #2 in particular. boost/process/detail/windows_helpers.hpp: Can you #include <cstring> instead of <string.h>? General: The #include <windows/unistd.h> is not consistent. Sometimes there's an extra #else # error "Unsupported platform." #endif sometimes not. In Christ, Steven Watanabe