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.
- 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...
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. 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::vectorstd::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?
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
A fragment of a sentence I some how missed. 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::vectorstd::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