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

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Bjorn Reese (breese_at_[hidden])
Date: 2016-11-05 08:20:34

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

> 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) &&;
   };

> 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.

>> 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.

>> 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.

>> 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.

>> Tutorial / Asynchronous I/O: Consider including an example with
>> boost::asio::spawn as a way to obtain the yield context.
> I linked to the boost.asio part of the documentation. Though I
> personally like coroutines very much, it's more a feature for people
> already familiar with boost.asio, not one for people starting with
> boost.process.

The example is more or less useless because it pulls a yield context out
of the thin air. It should either be removed, or replaced with a working
example like this:

   boost::asio::spawn(
     io,
     [] (boost::asio::yield_context yield) {
       bp::system("my-program", yield);
     });

>> Reference / cmd: bp::cmd will parse the input string as a sequence of
>> space-separated arguments. Is it spaces or whitespaces? Is it possible
>> to have escapes for arguments that do contain spaces?
> Spaces. I don't know what you mean by the latter, but "foo arg 1 \"arg
> 2\"" will yield {"foo", "arg", "1", "arg 2"}. It's written in a note in
> design.

That is exactly what I was asking for.

However, the note in the Design Rationale section is rather vague, and
please be aware that users seldomly read the Design Rationale as it is
supposed to answer why the library is designed as it is, not how to use
it. Please put that in the reference documentation.