Subject: Re: [boost] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Vicente Botet (vicente.botet_at_[hidden])
Date: 2011-02-08 04:22:08
Ilya Sokolov-2 wrote:
> On Tue, 08 Feb 2011 00:09:25 +0500, Vicente Botet
> <vicente.botet_at_[hidden]> wrote:
>> 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
>> release() to transfer ownership" lets think to some kind of move
>> Does it means that the access to handles that had shared ownership will
>> access to an invalid handle after ownership has been transferred? It's
>> 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
>> that the underlying interface manages with non-typed handles, doesn't
>> 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
>> Until we need to store heterogeneous handles, I would prefer specific
>> 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
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
> 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?
>> 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
> Or, better yet, rename the namespace with plural 'processes'.
>>> From where I can get the constructor pid_type ? I think that this
>> constructor must be protected and the create_child function declared
> 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.
>> 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;
>> What about executable_to_program_name?
>> pipe is defined like
>> typedef boost::asio::posix::stream_descriptor pipe;
>> 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
>> 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.
>> 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
>> Is get_instance() really needed? If yes, is it the implementation thread
>> The single function that left is get_id(). What about moving it to the
>> namespace level and just remove this class?
> 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
>> 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?
>> 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
>> 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
> 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
const char** arguments() const
-- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-Process-library-starts-tomorrow-tp3263607p3275577.html Sent from the Boost - Dev mailing list archive at Nabble.com.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk