Re: [boost] [process] Formal Review starts today, 27 October

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-11-13 08:54:33

Am 13.11.2016 um 13:23 schrieb Bjorn Reese:
> On 11/07/2016 01:46 AM, Klemens Morgenstern wrote:
>
>> I didn't understand it that way, especially since he Bjorn mentioned
>> boost::thread. I'm a bit tired of this discussion, and Boris explained
>> very well, why the initializers-approach was chosen. As far as I
>> understand it he considers everything but args as attributes and wants
>> them all crammed into one type (or something like that) which would be a
>> horrific mess. I should know, I tried to build somethinke like that
>> (based on boost.process), just two ours ago - it doesn't work for a
>> proper process library, there are just too many properties to a process.
>
> "Horrific mess" is in the eye of the beholder.
I'm talking about the implementation, not of the usage. Please prove me 
wrong, but with an implementation not an usage-example.
If you find a solution which can be integrated into boost.process, I'll 
add it to the library.
>
> With the current API it is unclear which parameters are mutually
> exclusive (e.g. can I pass both a yield context and an error_code?)
> or what happens if I specify the same attribute multiple times like
> this:
>
>   bp::child c(command,
>               bp::std_out > p,
>               bp::std_out > "output.txt");
>
Well this will use the second pipe, though I grant you, that's not the 
best solution here - an error would be better. I could add an assertion, 
so there are no duplicates of that. On the other hand it is similar to that:

child_builder cb;
cb.std_out(p);
cb.std_out("output.txt");

So it's not that strange.

> and of course the argument overwrite issue mentioned by Lee, all of
> which demonstrates that the current API is error-prone.
>
> The discussion about attributes is a bit fuzzy because the library does
> not define what they are. After a closer look it appears that there are
> several categories of attributes crammed into the initializer list.
>
Well yes, they are not called attributes, but properties. The reason is, 
that I don't see a different between the arguments passed and all the 
other stuff that's passed. After all, at least of those properties, 
you'd call attributes, can in some cases even select a different 
executable (i.e. the environment variables).
> One such category is the status-reporting (e.g. error_code and yield
> context.) Here is another idea for an API: have separate functions for
> synchronous and asynchronous invocation.
That's not the same. std::error_code is used to avoid exceptions and is 
only used when the process is launched. On exit you don't get an 
error_code, but only the exit-code of the process; and another 
std::error_code in case something went wrong with waiting. So there's no 
std::error_code vs. yield_context, they do different things and it 
actually makes sense to use them together.
Now in case you use your error_code with system, like that

bp::system("foo", ec, yield);

ec being set will indicate that the process was not launched 
successfully and hence the coroutine wasn't suspended.

Also, you do not need to pass an io_service when passing a 
yield_context, since the system function can get the io_service from the 
yield_context.
The io_service is needed bp::on_exit and the convenience stuff for 
asynchronous read, e.g. std_out > buffer(thingy).

Additionally you do not need to pass an io_service for async-operations 
to bp::system; if none is passed but a property requires that, system 
will internally create one and use that.

> Let the synchronous invocation accept one command parameter, one
> optional attributes parameter, zero or more arguments, and one 
> optional error_code. The latter determine whether the call throws or 
> not. I am
> deliberately omitting the attributes parameter below because it is
> irrelevant to the sync/async split.
>
>   bp::system(command, arguments...); // throws
>   bp::system(command, arguments..., error_code&) nothrow;
>
> Let the asynchronous invocation accept one io_service parameter, one
> command parameter, one optional attributes parameter, zero or more
> arguments, and one handler/extension parameter (i.e. no error_code
> parameter.)
>
>   bp::async_system(io, command, arguments..., [](error_code){});
>   bp::async_system(io, command, arguments..., yield);
As stated above, both do not need to get an io-service passed, so I 
don't see the point of the second one. The first can already be done, if 
you want that, in the followin way:

bp::child(ios, exe, args..., on_exit=[](int code, const std::error_code 
&ec){}).detach();

I wouldn't recomment that, but that's possible. And no, bp::spawn 
doesn't work.