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-10-31 08:17:05

Am 31.10.2016 um 01:18 schrieb Gavin Lambert:
> On 31/10/2016 04:39, Klemens Morgenstern wrote:
>> Am 30.10.2016 um 14:21 schrieb Bjorn Reese:
>>> The bp::child object should not execute in the constructor, but rather
>>> via operator(). That way we can use the named parameter idiom to build
>>> up redirections and similar modifications. For example:
>>>
>>>   auto compile = bp::child("c++").output(bp::null);
>>>   compile("main.cpp"); // Executes
>
> +1 for me for that style too.
>
>> I personally don't like that approach though it certainly would work. I
>> think it's partly a matter of taste, here's my (C) association:
>>
>> struct X {int std_in; const char* exe;};
>> X x{.std_in = 2, exe ="stuff"};
>>
>> I.e. your setting properties.
>>
>> Now the problem I see is this: what happens here?
>>
>> auto compile = bp::child("c++");
>> compile.output(bp::null);
>> compile("main.cpp");
>> compile.env({"PATH", "/foo"});
>
> The fourth call should fail (and probably throw), since it's impossible
> to change the environment after the process has been started.  Otherwise
> that should work as you'd expect.
>
> One of the great advantages of this style of construction is that it
> permits conditionals in a fairly natural fashion:
>
> auto compile = bp::child("c++");
> if (quiet)
>     compile.output(bp::null);
> compile.arg("-c");
> if (debug)
>     compile.arg("-D_DEBUG");
> compile.args("-DFOO", filename);
> for (auto lib : libraries)
>     compile.arg("-l" + lib);
> auto child = compile();
>
> or something like that.  Or you could have a factory function that
> returns the un-executed builder to the caller to add additional
> parameters before finally actually executing it, which aids in
> composability.
>
I want to back up my claims here, so for comparison, here you have the 
documentation of boost.process 0.3 and you can see how many parameters 
you can actually set in the way you described: 
http://www.highscore.de/cpp/process/#creating

Five. Five options is what you have. And no asynchronous I/O.

If you look over 0.5 (with a variadic interface, 
http://www.highscore.de/boost/process0.5/boost_process/tutorial.html) 
you have this set of parameters, some of them mutually exclusive (like 
set_cmd, set_exe/set_args:

  - bind_stderr
  - bind_stdin
  - bind_stdout
  - bind_fd
  - close_fd
  - close_fds
  - close_fds_if
  - close_stderr
  - close_stdin
  - close_stdout
  - hide_console
  - inherit_env
  - notify_io_service
  - on_exec_error
  - on_exec_setup
  - on_fork_error
  - on_fork_setup
  - on_fork_success
  - on_CreateProcess_error
  - on_CreateProcess_setup
  - on_CreateProcess_success
  - run_exe
  - set_args
  - set_cmd_line
  - set_env
  - set_on_error
  - show_window
  - start_in_dir
  - throw_on_error

And this is without built-in support of asynchronous I/O or /dev/null. 
I'm very convinced that it is a bad idea to put all of this into one 
class by default.

>> So in order to do that, we'd need a child_builder class, that can be
>> converted, similar to boost.format. I consider this style pre C++11,
>> i.e. before variadics. Now granted: it seems not very C++ish, but it's a
>> very unique problem: most properties are inaccesible after you launched
>> the process, which is the reason they are not members.
>
> Variadics are cool, but they contribute to code bloat, since each unique
> sequence of parameter types requires generating a new overload, and they
> require templates, which precludes private implementation.
>

Alright to back my claims up a bit further, here's how a boost.fusion 
iteration looks like with O1: https://godbolt.org/g/2q4il0

Now granted, boost.process will have a few moves in there, but that's 
basically as small as it gets. The operator() mostly calls the syscall 
directly. Sadly the is not as readable when using boost.process.