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