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-05 09:47:18

Am 05.11.2016 um 13:20 schrieb Bjorn Reese:
> On 10/30/2016 04:39 PM, Klemens Morgenstern wrote:
>
>> 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"});
>>
>> 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
>
> That is an odd assessment. Just because something existed before C++11
> does not mean that it is outdated. For instance, this idiom is used
> heavily in the C++11 library sqlpp11:
>
>   https://github.com/rbock/sqlpp11
>
Well I'm not the author of this library so let me explain why I consider 
it inferior. You have to possibilities to do that; either you're just 
chaining up sequential calls as for example in boost::program_options; 
then I'd be all for it - even with more functions. I.e. in that version 
you have all the data already in the object your just setting it for 
convenience. I would be against that since every possible option would 
need to be put into this option class. This would incurr a massive 
runtime and compile-time (!) overhead, which I would not feel 
comfortable with at all.
Now the other option is (as in format) that you have an operator but you 
need a certain amount of calls to get the thing right, and that's what I 
consider outdated. I.f. "boost::format("%1, %2") % 42" yields an object 
of boost::format, that has an invalid state unless you use another %. I 
think the better style there is "boost::format("%1, %2", 42, "foo");" 
and get an error if you pass the wrong number of arguments.
Concerning this library, you have some cross dependencies that come into 
play (despite the already mentioned overhead), so that one argument 
might depend on another one. Thus I would have to check that at runtime, 
which adds even more overhead. That's the reason I think this approach 
is outdated; I don't mean that this is always the wrong choice, but it 
is as in boost.format a workaround for the lack of variadic arguments 
and that is what I meant.
>> very unique problem: most properties are inaccesible after you launched
>> the process, which is the reason they are not members.
>
> You can achieve the same for named parameters using ref-qualifiers.
> Something along the lines of (notice the && at the end)
>
>   class child
>   {
>     child& env(std::string key, std::string value) &&;
>   };
>
That's not a solution, because that would disallow something like that:

child c("gcc");
c.env("PATH", "/foo");
c("main.cpp"); //now launch the thing.

which was the main thing requested in an alternate syntax, and which I 
can see the benefit of.
So you'd be replacing

child c("foo", args+={"bar"}, std_out > pipe, std_err>null);

with

auto c = child("foo").args("bar"). std_out(pipe).std_err(null);

I don't really see the benefit in that.

>> The next problem is: your proposed syntax solves a very special problem,
>> how would I know if you want to pass the first arg (as in your example)
>> or the exe or anything else to the operator()? I would constraint what
>> you can express.
>
> Arguments (those that arrive at the child main in argv[1..]) should be
> passed as parameters, all other attributes as named parameters.
>
> That said, a formal review is not the best venue for a design
> discussion.
And it does only cover your use-case.. It makes sense for

child compile("gcc");
compile("main.cpp");

but doesn't help here:

child antlr("java");
antlr("-jar", "antlr4.jar", "language.g4"); //here I'd much rather have 
antlr("language.g4");
>
>>> The bp::starting_dir should be renamed to bp::current_path to be
>>> consistent with Boost.Filesystem.
>>>
>> Thought about that, but it's not the current_path, since the child
>> process can change it's execution directory.
>
> I am not sure I understand that argument. The attribute in question
> sets the current path of the child process at the time of invocation.
> Whether the child process changes that is irrelevant.
>
> Anyways, at the very least the name should use the _path suffix instead
> of _dir to be consistent.
It's not an argument in a discussion really, it's just what I though 
would make the most sense. I though it ain't CWD, i.e. current working 
directory, but it's the one it's starts in, hence start_dir. It's a 
directory not a path, because that might a path to a file.
>
>>> The library should throw its own process_error exception which inherits
>>> from system_error, similar to Boost.Filesystem.
>>>
>> Not sure what this would do, the boost.filesystem error gives you more
>> info about which paths are concerned. This is not true for this library
>> so I'd need a new exception class for every error, which would be a bit
>> of an overkill.
>
> You only need one for the library, and it is quite trivial to write:
>
>   class process_error : public system_error
>   {
>   public:
>     process_error(error_code ec) : system_error(ec) {}
>     process_error(error_code ec, const std::string& what) : 
> system_error(ec, what) {}
>     // and the remaining 4 system_error constructors
>   };
>
> It gives the application the ability to distinguish between system
> exceptions thrown from different libraries.
Oh ok, that's the reason, I thought you wanted more information. Yeah, I 
could add this very quickly.
>
>>> The documentation is lacking an entire chapter with an overall
>>> description of the library and the concepts used. For instance:
>>>
>> Well that's correct, but on purpose. It's a documentation meant to
>> explain the library, not system concepts. I chose names that correspond
>> to what the stuff is called in the system, and linked to the systems
>> documentation occasionaly. I don't think such a documentation is a book,
>> it should only explain what it does, not what the system does.
>
> The documentation should introduce the concepts it is using, and
> describe how they relate to the library.
>
> See the Overview chapter in Boost.Asio for an example of how it can
> be done. It is not an introduction to network programming, but it does
> explain the architecture and concepts used in Boost.Asio.
>
Well there seems to consensus that this needs to be added.