Re: [boost] Subject: Formal Review of Proposed Boost.Process library starts tomorrow

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:
> 
>>
>> 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-tt3225411.html
>>
>> [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-Process-library-starts-tomorrow-tp3263607p3275577.html
Sent from the Boost - Dev mailing list archive at Nabble.com.