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: Ilya Sokolov (ilyasokol_at_[hidden])
Date: 2011-02-08 05:33:57

On Tue, 08 Feb 2011 14:22:08 +0500, Vicente Botet  
<vicente.botet_at_[hidden]> wrote:
> Ilya Sokolov-2 wrote:
>>
>> 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

as implementation detail

> and stream_ends if I'm  notwrong. Couldn't we have two classes so we  
> don't use a stream handle to
> construct a process and vice-versa?

I'm fine with it as is, except of the name.

>>> Shouldn't the public constructor from a native_handle be reserved to  
>>> the
>>> implementation?
>
> Could you comment this.

IMO no need to be too clever/restrictive.

>>> 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?

Not sure what you mean by 'always', but in this specific case  
(GetStdHandle())
it is strictly not needed.

> we don't have a portable way to get pid_type and the user
> is not intended to use these constructors.

Why?

> 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,

or portable interfaces written by himself for his own purposes

> which we should avoid.

No

>>> [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?

Almost (at least async_[read/write]_some are identical)

>>> If not it is not good to give
>>> them the same name, by the same reason asio has not done it?

It is hard to convince            windows/posix           guy
to name something       stream_descriptor/stream_handle, accordingly ).

> 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.

I'm fine with or without 'native_' prefix.

>>> 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?

I don't know anybody who wishes to remember quoting rules.

>>> [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.

Yes.

>>> 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?

Yes, and passing proper separators, quotes, etc to split_unix().

>> 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

Who will own the result?