On Tue, 08 Feb 2011 00:09:25 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Hi,
[snip]
[handle] handle is more than RAII, it tracks shared ownership to the native handle but this is not stated clearly on the doc. The sentence "user needs to use release() to transfer ownership" lets think to some kind of move semantics. Does it means that the access to handles that had shared ownership will have access to an invalid handle after ownership has been transferred? It's like if we had a shared_ptr<unique_ptr<>>. Do we really need shared ownership? Isn't unique ownership enough?
It is enough, I think, but we still don't have move-enabled containers.
What is behind a handle, a process handle or a stream end handle? The fact that the underlying interface manages with non-typed handles, doesn't means that the C++ interface must follows the un-typed form. We can define a strong-type interface to avoid bad uses that will result in run-time errors. Until we need to store heterogeneous handles, I would prefer specific types for each type of handle.
Boost.Process uses bp::handle publicly as file (descriptor/handle) only. Could we rename it to 'file' then?
Shouldn't the public constructor from a native_handle be reserved to the implementation?
What about using the bool idiom conversion instead of valid()?
If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle?
Nobody in some cases, and that is not a problem (for the result of GetStdHandle() function, at least).
[snip]
[process::process] How can I create a process instance? From where I can get the pid_type or the handle? I think that these constructors must be protected.
What about renaming it base_process to avoid ambiguities with the namespace.
Or, better yet, rename the namespace with plural 'processes'.
[child]
From where I can get the constructor pid_type ? I think that this constructor must be protected and the create_child function declared friend.
That will increase coupling.
[pid_type] Which are the operations that can be applied on this type? What about renaming it to native_pid_type, so the user see that he is manipulating a non portable entity.
I suggest to drop process::id_ member of type pid_type and add process::native_ of type native_type, typedefed to int/HANDLE;
[snip]
typedef pipe async_pipe;
Does it means that on POSIX systems pipe and async_pipe is the same?
Yes
[executable_to_progname] What about executable_to_program_name?
[pipe] pipe is defined like
typedef boost::asio::posix::stream_descriptor pipe;
or
typedef boost::asio::windows::stream_handle pipe;
bp::pipe is documented as 'type definition for stream-based classes defined by Boost.Asio', so why it can't be named 'asio_stream'?
Do these classes provide the same interface? If not it is not good to give them the same name, by the same reason asio has not done it?
[std_stream_id/stream_id]
What about defining an opaque class that accepts only the values stdin_id, stdout_id, stderr_id on Windows and more on Posix systems?
[snip] - I answered this on another reply.
[self]
How self().wait() behaves? Blocks forever? throw exception?
We have already ::terminate(). Why do we need self().terminate()?
Doesn't Boost.Filesystem provides already get_work_dir, current_path? How do you change to another directory? rename get_work_dir to get_working_directory?
What about Boost.ProgramOptions environment_iterator class instead of get_environment()?
Is get_instance() really needed? If yes, is it the implementation thread safe?
The single function that left is get_id(). What about moving it to the namespace level and just remove this class?
+1. BTW, bp::self::get_work_dir() relies on BOOST_PROCESS_POSIX_PATH_MAX, but boost::filesystem::initial_path() doesn't.
Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt...
[snip]
Windows users will prefer the set_command_line function.
I doubt it ), IMO one interface is enough.
[snip]
User that write portable programs would need to choose between one of the two ways to pass the arguments. Of course the program could use some conditional compilation that could use the most efficient.
If the user uses the add interface on Windows, the implementation will be as efficient as now.
No need to be efficient here, as all savings will be eaten by the CreateProcess() call.
If the user uses the set_command_line on c-like systems, the class need to tokenize the arguments.
(split_winmain() and split_unix() in boost::program_options)
[snip]
See my alternative implementation in the attachment.