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-10-27 13:41:17

Am 27.10.2016 um 10:47 schrieb Johannes:
> 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.
>
Thank you for your review. It was surely a mistake to only read the 
sources of the doc for errors, so I missed too many things; I'm really 
confused why so many links didn't work. I fear I cannot change the 
documentation during the review, but you're certainly right, it is a 
weak point. Nevertheless, I'll go through and address your points, so 
other reviewers might have some questions answered.
Despite the sloppy errors in the doc, it really helps me, because I am 
obviously very deep in the topic, so I don't always knwo what needs 
explaining.
> > - 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?
Yeah I should, especially because it's platform dependent. There's a bit 
more doc to that here: 
http://klemens-morgenstern.github.io/process/boost_process/design.html#boost_process.design.arg_cmd_style 
though that doesn't explain the differences in the implementation.

>
>
> 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 ...'?
>
The exit code.

> Unclear: 'As with all other functions, passing an std::error_code will 
> change the behaviour, so that ...'
>     Whar are all these other functions?
child::child(), system, launch
>
> 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?
No I mean std_err, my bad.
>
>     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.
>
The latter actually works ( with bp::null that is), and it's because you 
redirect one stream. You of course have to pass it to system, because 
you have to set this all up before launching the process. But sure, that 
should be explained.
>
>
> 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?
Well it is actually not necessary because the function "running" takes 
care of that. But that is in fact not clearly documented, I'll do that.
>
> Unclear: '...and with [globalname boost::process::std_out std_out] by 
> using [classname boost::process::opstream opstream]]'
>     What are these?
>
A fragment of a sentence I some how missed.
> Because of knowledge of tool 'nm' I do not understand the pipe example 
> 'bp::child c("c++filt", std_out > out, std_in < in);'
Thanks for the remark; I actually assumed every C++ developer would know 
the gnu toolchain, which would make this the perfect example.
Nm reads the outline (i.e. every entry point) of a binary and prints 
them into stdout.
>
> 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.
>
It takes a mangled name and outputs the demangled name.
> 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.
I see, yes I assumed the knowledge of what a pipe is. A pstream wraps 
aronud a pipe, but since p is only used to forward it, it doesn't need a 
stream around it. I'll add more information there.
>
> 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.
>
Sure thing, or at the very least link to the reference.
> 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.
>
Yes, and I'll add links to the actual implementation, which are job 
objects on windows and process groups on posix.
> 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.
>
Here I do in fact assume prior knowledge, that's why it's linked to the 
boost.asio coroutine stuff.
> Best,
> Johannes
>
Liebe Grüße,

Klemens