Ilya Sokolov-2 wrote:
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.
Great to see that unique ownership could be enough. We will have move-enabled containers soon with Boost.Move and Boost.Containers, at least I hope so.
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?
The handle class is used as parameter of process and stream_ends if I'm not wrong. Couldn't we have two classes so we don't use a stream handle to construct a process and vice-versa?
Shouldn't the public constructor from a native_handle be reserved to the implementation?
Could you comment this.
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).
Do you mean that the close of the handle is not really needed in this specific case or always?
[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.
You are right, but we don't have a portable way to get pid_type and the user is not intended to use these constructors. If this is a public interface, the user could be tempted to use them and will need to get the pid_type using non portable interfaces, which we should avoid.
[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;
Great.
[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?
Could you comment this? IMHO any class that has a specific interface should be prefixed with native as the user could need conditional compilation that depends on the platform to use it.
[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.
Great.
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.
We disagree here ;-) Why the container based interface would be always better than the command_line?
[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.
You mean we don't need to be efficient here, isn't it? You are right, the time taken by the process creation is some order magnitude the time can be spared.
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)
Do you mean that the user can use these functions in his application, using conditional compilation?
See my alternative implementation in the attachment.
This is better, but it forces the implementation to return the arguments as a std::vector<std::string>. I would move the functions that make the decoding and let the interface return a char**, that is what the implementation needs. const char** arguments() const Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.