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

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Johannes (JBrunen_at_[hidden])
Date: 2016-10-27 04:47:44

Hello,

following you can find my little review of the process library. 
Unfortunately, I have no more time to dive into more details of the 
library. But from what I have read and seen on the list I think that the 
library would be a good addition to boost. The author seems to be quite 
knowledgeable about the library domain and I think that he will  bring 
it into the state that will be consensus after the review.

 > - Whether you believe the library should be accepted into Boost
 >   * Conditions for acceptance
Yes, I think that boost should have this process library.

Disclaimer: I only read the tutorial and have judged from the point of
view, whether I would like to use that library in my code or not.

 > - Your knowledge of the problem domain
Simple user level only.

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

 >   * Implementation
Did not look into it.

 >   * Documentation
I would like to see that the author spend some more time into it. 
Especially, the tutorial which basically is the attempt to sell the 
library to me should be expanded (sell below). I would also like to see 
some advise for best practice when working with processes.

That the examples use not commonly known tools for illustration 
(nm,c++filt) has not eased the reading of the tutorial. The author 
should introduce the tools so that the reader has some familiarity with 
their behavior.

 >   * Tests
Did not look into these.

 >   * Usefulness
Very useful. This is an important library and the boost should have one.

 > - Did you attempt to use the library? If so:
No, I only read the tutorial.

 >   * Which compiler(s)
N/a

 >   * What was the experience? Any problems?
N/a

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

Following the notes I have taken during studying the tutorial:
Spelling error: 'Wwe' in Tutorial/Starting a process

Sentence: 'Now, there is a subtle difference between the two syntaxes, 
i.e. passing a single string or passing one.'
     Should be '..., i.e. passing a single string or passing several 
strings.'

Puzzling: '... when passing one string it will be interpreted as a 
command. This might lead to different results which are platform dependent.'
     Could you elaborate?

Spelling: 'The spawn function launches a process and immediately 
detaches so, so no ...'
     Should be '... detaches it, so no ...'

Unclear: 'The call to wait is necessary, to obtain it and tell the 
operating system, that no one is waiting for the process anymore.'
     What is meant by 'it' in '..., to obtain it ...'?

Unclear: 'As with all other functions, passing an std::error_code will 
change the behaviour, so that ...'
     Whar are all these other functions?

Preferable: I would like to see bp::null_device instead of bp::null

Hmmm: With respect to 'bp::system("g++", "main.cpp", bp::std_out > 
"gcc_out.log");'
     In the reference 
http://klemens-morgenstern.github.io/process/boost/process/std_out.html 
you write:
         'The Semantic is the same as for std_out'
     Do you mean std::cout or unix shell standard output?

     The syntax is a little foreign to me. Actually, I would have 
expected something in the line of...
         'bp::system("g++", "main.cpp") > bp::null_device;'
         'bp::system("g++", "main.cpp") > "gcc_out.log"'
     or
         'bp::system("g++", "main.cpp", bp::null_device);'
         'bp::system("g++", "main.cpp", "gcc_out.log");'
     or
         'bp::system("g++", "main.cpp", bp::std_out = bp::null_device);'
         'bp::system("g++", "main.cpp", bp::std_out = "gcc_out.log");'

     At least, you could elaborate somewhere, why you choose your 
syntax. It is not self explaining and should be
     introduced in more detail.

Spelling: 'Every entry point ... At the end and empty line is appended ...'
     Should be 'At the end an empty line is appended'?

Sentence: 'Boost.process provides the pipestream ... and provide an 
implenentation ...'
     Shouldn't it be '... which provide an implenentation ...'

Example: 'std::vector<std::string> read_outline(std::string & file)'
     I did miss the c.wait() call. Is it not necessary here and if so why?

Missing link: 'You can do the same thing with [globalname 
boost::process::std_err std_err '

Unclear: '...and with [globalname boost::process::std_out std_out] by 
using [classname boost::process::opstream opstream]]'
     What are these?

Because of knowledge of tool 'nm' I do not understand the pipe example 
'bp::child c("c++filt", std_out > out, std_in < in);'

Unclear: You told me about child::wait and its variants and mentioned 
child::detach but now you use child::terminate.
      I wished you would explain these functions in the tutorial and not 
expect me the look up the reference here.
      Actually, the reference does not really tell me anything: 
'Terminate the child process.' What does process termination
      mean. What is the state of the child instance that I have at hand? 
Can I asked if a child is actually terminated?

Hmm: Another unknown tool 'c++filt'. Please introduce these tools a 
little more. Not everyone has an unix toolset background.

Example: Again you are not working with child::wait anymore.

Unclear: In the example 'std::vector<std::string> 
read_demangled_outline(std::string & file)' you introduce the class 'pipe'.
     and use it in 'bp::child nm("nm", bp::std_out > p);'. After what I 
have read until know in the tutorial I did have
     expected that you would come up with a iopstream object in analogon 
to the std::iostream. Give some background to the 'pipe'
     class for sake of clarification. You have room in the tutorial to 
take your customers at hand.

Link:  [http://www.boost.org/doc/libs/release/libs/asio/ boost.asio] two 
times.

Example: Second async example: You note that the child::wait call is 
superfluous. I would not use child::wait in the example
          because you explain it right in the note that is not necessary.

Example: Async example with std::future. That example is not self 
explaining.
     'bp::std_in.close()' ?
     'bp::std_out > bp::null, //so it can be written without anything' ?
     'bp::std_err > data' ? A'm I only interested in errors in this example?

     Please give more room for explanations. You are selling your 
library in the tutorial.

Unclear: 'This will also apply for a child process, that launches other 
processes, if they do not modifiy the group membership. ...'
     I did not understand your sentence here. You need to teach more 
about groups and what can be done with them.

Link: [globalname boost::process::env env]

Unclear: 'Stackless coroutines can be implemented rather easily, so...'. 
So you are expecting from me in the tutorial that
     I'm familiar with the coroutines concept. No I'm not. Please take 
your time to provide me with enough background information so that
     I might be willing to invest resources to tackle the coroutine 
concepts and use it with your process library.

Best,
Johannes