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 03:30:11

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.

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