On Mon, 07 Feb 2011 22:06:09 +0100, Steven Watanabe <watanabesj@gmail.com> wrote:
[...]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:
Thanks, all changed locally.
Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info?
As Ilya had noticed I took the macro from him. I'll check again whether we can replace it with BOOST_THROW_EXCEPTION.
[...]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.
Ok, I'll check the ticket.
[...]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.
I'm fine changing it.
[...]create_child: [...]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.
Makes sense - I'll look into it!
[...]Can you #include the appropriate specific header, rather than the whole boost/asio.hpp?
Also noted (after Artyom's mail I'd appreciate more feedback though if support for asynchronous wait operations should be dropped).
[...]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.
The implementation gets so complicated that I wonder whether this gets out of proportion. Even if all of this works at some point a developer could always create a thread himself and call wait(). That's not cross-platform and not as convenient as calling async_wait(). But I can fully understand that Artyom is not happy with the implementation. And it would get even more complicated. :-/
[...]boost/process/detail/windows_helpers.hpp:
Can you #include <cstring> instead of <string.h>?
I think MSDN says to include string.h for those _s functions like memcpy_s. But then you are right that cstring should probably work, too (will test). Thanks again, Boris