Am 14.09.2016 um 16:29 schrieb Klaim - Joël Lamotte:
On 12 September 2016 at 14:41, Giel van Schijndel < Giel.vanSchijndel@tomtom.com> wrote:
* You assume the presence of $PATH and a current working directory. Unfortunately not all operating systems have these concepts: notably Windows CE has neither environment variables nor a current working directory. * Additionally current working directory duplicates functionality from Boost.Filesystem * You seem to always take paths (on Windows) by 'char' based strings. This will be problematic in that the Windows ("ANSI") API interprets these dependent on the current code page. Providing the ability to use 'wchar_t' based strings (or just boost::filesystem::path) should alleviate that problem because it's always UTF-16. Additionally, Windows CE, in the incarnations that I've seen, doesn't even provide the "ANSI" API, meaning the Unicode API is the only option.
I share these concerns.
It's not easy to solve, it would probably look like this: child c("äöü", windows::unicode); I do however think, that such cases are rather rare for processes, while they might be regular for filesystems.
Also here are comments I wrote while reading the current documentation in details:
Thank you very much, that really helps. I commented a few points; but you're right I should link to more, that will help. Other than that, I'll work your comments into the documentation.
Boost.Process Documentation Review ============================
In reading order:
1. Arguments/Command Style: Could you clarify if there are different semantics for cmd and exe/args styles? I assume that it is purely syntactic sugar for C++.
That's actually written there: "The cmd style will interpret the string as a sequence of the exe and arguments and parse them as such, while the exe-/args-style will interpret each string as an argument." It's actually not syntactic sugar, because the call of CreateProcess is different on windows. But yeah, I should add this.
4. Starting a process: It is not clear to me what character types are supported by system(). Also, in general, what are the advantages of using boost::process::system over std::system?
Uhm, you can path any of the properties. I thought that was obvious by the examples. It's basically an extended std::system.
5. I had a hard time finding reference docs to the different names in the tutorial, because there is no link in the functions and classes names.
6. Does Boost.Process process starting functions allow passing boost::filesystem::paths or anything that looks like it?
You can use this as a parameter for "exe", and for I/O. I.e. you can to
this:
fs::path log="log.txt", out="out.txt", in="in.txt", _exe="stuff.exe";
child c(exe=_exe, std_out>out, std_in
8. Why not name the null device null_device instead of null? "null" looks potentially misleading.
Possibly, but it's shorter. Also, since nullptr is a keyword, what would it lead you to think? Btw: I orginally wanted to use nullptr here, but that would lead to confusion between closing & redirecting to null.
9. First example in I/O: maybe I'm wrong but it looks like the last line can never be read. Or maybe I do not understand when exactly does c.running() returns false. Or maybe I'm thinking with windows console that can, if I remember correctly, continue printing output while the command's process is already finished.
10. In the same example, isn't there a race between the c.running() and the other conditions? What if it becomes false after being called, but before calling getline? Same question for the second example.
I think you're right. I know why I don't use Sync I/O.
12. In the example, am I correct that
cout << fut.get() << endl;
should be
cout << data.get() << endl;
Yes you are.
15. I would prefer you to clarify in this section that this implementation of terminate() will not notify the child process (through C signals on posix or other api on windows) before ending it, and if the user want such notification, point to other some other doc about this subject.
Oh, well that will be frustrating, because I can only point to a section where I explain why that's not possible. Would make sense for the review though.
18. The pipe section makes me wonder: if each time you use a synchrounous pipe you enable a potential deadlock which is basically unavoidable, why even allow it? Why not allow only the async_pipe?
That's due to the way the OS are working. The deadlock is not specific to the library. It makes sense because you might have an application where boost.asio is an overkill and you know how the output looks.
22. " It essentially works like a shared_ptr, though several copies may not represent the changes done by one." So...it does not work at all like a shared_ptr?
Uhm, yeah, that's bad. It points to the same shared data, but you need to renew it manually. At least on windows.
25. Is there a reason why pwd() and path() doesn't return boost::filesystem::path objects?
I already removed them because they're also in boost.filesystem. It did return std::string because it was a const char* from the system.
26. What is a property? When/where can they be used? It is not explained at all. I can guess from the rest of the examples and my experience with other boost libraries but it's still not super clear to the newcomer.
Well a property is something like "exe", a property of the process. Since you brought that up: I didn't want to call it a parameter, because that could get confusing since parameters build properties, i.e. "system("gcc", args="thingy.c", args+="-o", args+="thingy.o");", which would define two properties "exe" and "args". I couldn't call them members though, because they are not visible class-members.
27. Are there incompatibilities between the different properties? Or are they all combinable?
Execept for exe-args vs. cmd completely free.
29. "The throw_on_error property will enable the exception when launching a process. It is unnecessary by default, but may be used, when an additional error_code is provided."
I do not know at all what this is about. Maybe develop?
Yeah, inherited from process 0.5. It's basically there if you want dual error handling. This can be useful for further development, but maybe should not be documented.
30. io_service and throw_on_error sections lacks a simple code example.
throw_on_error should be removed and io_service does nothing on it's own. It's in the asio examples though.
31. The I/O syntax needs to be explained. I'm talking about (something & somethingelse) > text Instead of relying on just an example, clarify - where this syntax can be used; - what should be where (in the parenthesis? are the parenthesis necessary? what is right/left of the arrow?) - if it imply some special semantic;
Ok, thanks. This is what you do in the shell "stuff &2> log.txt", so you combine both out streams to one sink. It's only available for "(stderr & stdout) > sink".
32. Why is there 2 different Child sections? Why are they separate?
Because one is the class description, the other is the launch mode.
33. What is the equivalent process launcher function that the child constructor's behaviour matches?
There is none, that's why there's a custom section for this. The child ctor itself does all the work, so I don't see the need to have a launcher function.
--------
Note that I didn't try yet to use the library, but intend to before the review week.
Awesome!
Joël Lamotte
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost