Re: [boost] [gsoc] Boost.Process done

Subject: Re: [boost] [gsoc] Boost.Process done
From: Ilya Sokolov (ilyasokol_at_[hidden])
Date: 2010-09-07 21:37:52

On Sun, 05 Sep 2010 18:05:36 +0400, Boris Schaeling <boris_at_[hidden]>  
wrote:

> As there has been a lot of feedback I started to update the library.  
> There is now a new version available at  
> <http://www.highscore.de/boost/gsoc2010/process.zip> (and of course in  
> Subversion at  
> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/process/>).
>
> What has been changed?
>
> [snip]
>
> I'd appreciate if you could comment on the changes. Especially have a  
> look at:
>
> * this signature: std::pair<handle, handle> (bool).

It would be better if delegate would accept fileno instead of bool
indicating input/output. bp::behavior::inherit seems almost
indistinguishable from the redirect_to.

> Shall we use std::pair<handle, handle> to return two handles for the  
> child and parent process or a struct with member variables called  
> parent_end and child_end? Shall we use a bool to indicate whether an  
> input or output stream is configured or something else like an  
> enumeration? I think it all depends on how many developers want to  
> define new stream behaviors and if it's worth to make the signature a  
> bit more self-explanatory?
>
> * the implementation of async_pipe on Windows. Are return values of  
> Windows functions checked correctly?

Nod.

> Is the return value of UuidCreateSequential() and UuidToStringA() the  
> error code or must GetLastError() be called?

The former, and it should be passed to the system_error's constructor.
system_category should be fine for it.

> Is it OK if we require developers to link against rpcrt4.lib on Windows
> because async_pipe uses UuidCreateSequential() now?

Nod.

> * the example at  
> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/example/redirect_to.cpp>.  
> It uses a user-defined behavior class to redirect the child's standard  
> error stream to the standard output stream. As the behavior of the  
> standard error stream must somehow access the handle of the standard  
> output stream I create the pipe myself before I set the stream  
> behaviors. As the pipe must be used for the standard output stream  
> though I define a forward function which I can bind to the respective  
> delegate. Anyway, please have a look at the example and think about  
> whether it makes sense or if there is anything to simplify. :-)

IMO, the redirect_to_stderr behavior must be provided by the library
as it is a common use case. Unfortunately, it breaks the interface (((.

> What still has to be done?
>
> * The context class still has a default constructor only which  
> initializes the stream behaviors to inherit standard streams. This is a  
> problem for Windows GUI applications as they don't have standard streams  
> (or at least they are closed). While the Named Constructor Idiom has  
> been proposed what exactly should the done? Should the default  
> constructor be private and developers must explicitly use a factory  
> function to create a context object? Shall the factory function  
> initialize all standard streams with the same behavior? Shall we provide  
> a factory function which expects three parameters to initialize the  
> standard streams? What's convenient and flexible and not too confusing?

I expect that if some std stream is closed in the calling process,
the default constructor of the context class doesn't throw anything.
As I see it, the bp::handle should have a "dont-close" flag and
bp::behavior::inherit shouldn't call dup() or DuplicateHandle().

> [snip]