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-10-30 09:21:36

On 10/27/2016 08:26 AM, Antony Polukhin wrote:

> - Whether you believe the library should be accepted into Boost
>   * Conditions for acceptance

I withhold judgement for now to give the author an opportunity to
respond. At the moment I am unsure whether to vote for conditional
acceptance or rejection.

> - Your knowledge of the problem domain

Advanced on Unix.

> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
>   * Design

I have a couple of serious concerns.

My first concern is about the use of tagged arguments, and especially
the redirection operators, in constructors.

Instead I propose that the library uses the Named Parameter idiom (see
[1], not to be confused with what Niall labelled "tagged name parameter"
idiom.)

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

My second concern is about deadlocking. The FAQ shows two ways of
deadlocking. I would expect the latter (the ls example) to work
without further ado because that is how any newcomer would write
their code.

My third concern is about the chaining of pipes between processes. The
tutorial shows an example where the output of the nm command is
forwarded to the c++filt command. However, the forwarding is not done
directly from the nm process to the c++filt process, but goes via the
parent process. This can incur a performance degradation when nm
generates much output.

Is it possible to chain them directly? Besides "nm myfile | c++filt"
that is.

I also have the following minor comments / questions.

Is it possible to redirect stderr to stdout? (like 2>&1). The reference
documentation only mentions this under asynchronous pipe output.

The bp::starting_dir should be renamed to bp::current_path to be
consistent with Boost.Filesystem.

Consider renaming bp::spawn to avoid confusion with boost::asio::spawn
which is how you launch coroutines with Boost.Asio.

I am missing a this_process::get_id() function to return the current
process identifier.

>   * Implementation

My major concern with the implementation is that the library is only
header-only, so I cannot compile it as a static library if I wish to.
There are several Boost header-only libraries that allows you to do
that. The reason why I find this important for Boost.Process is because
I want to avoid getting platform-specific C header files included into
my project when I include a Boost.Process header.

The library should throw its own process_error exception which inherits
from system_error, similar to Boost.Filesystem.

I did not investigate the implementation in greater detail.

>   * Documentation

The documentation is lacking an entire chapter with an overall
description of the library and the concepts used. For instance:

   * What are processes and pipes?

   * What are the possible invocation mechanisms? Some of this is
     mentioned in the Design Rationale, but that is not the place
     where developers seek their information.

   * How to set search paths or the current working directory.

   * How to use environment variables.

   * this_process

Tutorial / Synchronous I/O: What is the default behaviour when I/O
redirections are not specified?

Tutorial / Asynchronous I/O: Consider including an example with
boost::asio::spawn as a way to obtain the yield context.

Reference / child: Does not mention how the life-time of the object
and the child process are related. For instance, the documentation on
the destructor only mentions that it is a destructor.

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?

>   * Tests

Did not investigate.

>   * Usefulness

A process library is a very useful addition to Boost.

> - Did you attempt to use the library? If so:
>   * Which compiler(s)
>   * What was the experience? Any problems?

Did not try.

> - How much effort did you put into your evaluation of the review?

About 4-5 hours (one of which was gratuitously donated by daylight 
saving time.)

[1] https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Named_Parameter