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-07 14:09:25

Hi,

for the moment I have started with the documentation. Here there are some
remarks and questions:

I would prefer to see in the Synopsis which functions are available on which
specific platforms. Instead of
  
  process(handle);

document 

#if defined(BOOST_WINDOWS_API)
  process(handle);
#endif

so it will be more clear what can be used.

It is not documented which exceptions can be thrown, if any. Should a lib a
Boost.Process use the Boost.System library and report failures following a
coherent approach?

I encapsulate together the executable name and the arguments in a
command_line class.

The documentation must describe explicitly the requirements of the used
concepts.

[Context]
Requirements must be documented

[Arguments]
Requirements must be documented and allow efficient implementations. (recall
my post [process] Arguments and Context] see bellow)

[environment/context::streams_t]
This class is too concrete. I would prefer to have a concept behind and that
allow efficient implementations.

[StreamBehavior]
Requirements must be documented

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

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. 

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?
What happens when it closes the native handle and we access to one of this
orphaned handles?

private functions like invalid_handle don't need to be documented. Shouldn't
the nested impl class be private?

I don't know if we can have a better design by separating the handle class
into the handle itself and a class that provides the kind of ownership we
need to have (either shared either unique)

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

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

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

Can pid_type be shared between process?

[pistream/postream]
class pistream : public std::istream, public boost::noncopyable

* inheritance missing on the documentation.
* constructor must be documented explicit

[status]
inheritance missing from the documentation

: public boost::asio::basic_io_object<Service>

The reference interface don't show how it can be constructed?

[async_pipe]
The documentation shouldn't show this typedef as in windows it is not the
case.

typedef pipe async_pipe;

Does it means that on POSIX systems pipe and async_pipe is the same?

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

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?

class stream_id {

  enum type { stdin, stdout, stderr };
  #if defined WINDOWS
  typedef type value_type;
  #elif defined POSIX
  typedef int value_type;
  #else
  #error
  #endif

  stream_id(); // incalid id
  stream_id(value_type v); // check for negative values on Posix
  #ifdef POSIX
  stream_id(type v); // don't need to check
  #endif
  operator value_type();
  ...
};

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

Best,
Vicente

Extract from post [process] Arguments and Context concepts 
http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt3225411.html

The problem I see is that you are using std::string in the public data types
of the fields, which avoids to have an efficient implementation and
requiring containers that have nothing to be with the ones needed by the
underlying platform interface.

The current function to create follows this prototype:

template<typename Arguments, typename Context>
child create_child(const std::string & executable, Arguments args, Context
ctx);

Arguments must be a container of std::string, and the process_name will be
inserted at the beginning of the container if given explicitly on the
Context.
Context has 3 fields that depend on std::string
  std::string process_name;
  std::string work_dir;
  environment env; env field must have as type an associative container
mapping  std::string to std::string.

I would move the process_name data to the Arguments concept. For what I have
see on the implementation there are two main way to pass arguments at
process creation:

C-like: using const char** args
Windows: Using a const char* command line following a specific grammar

I would try to abstract both strategies in something like

template <std::size_t N=0>
struct arguments {
    arguments();
    arguments(const char* process_name);
    ~arguments();
    const char* get_process_name() const;
    void set_process_name(const char* process_name);
    void add(const char* arg);
    const char* set_command_line();
    const char* get_command_line();
    std::size_t argc() const;
    const char** args() const;
private:    
    std::size_t argc_;
    const char* args_[N+1];
    const char* command_line_;
    bool args_must_be_released_;
    bool command_line_must_be_released_;
};

Users that use to work on C-like systems could use the add() function.

  arguments <2> args("pn");
  args.add("-l");
  args.add("-r");

  create_child(find_executable_in_path("pn"), args);

Windows users will prefer the set_command_line function.

  arguments <2> args();
  args.set_command_line("pn -l -r");

For these two use cases, the preceding class can be implemented in a way
that is as efficient as possible avoiding copies, allocations, releases.

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. If the user uses the set_command_line on c-like systems,
the class need to tokenize the arguments. Copies, allocation and releases
will be needed in these cases as it is done now.

The same applies to the Environment Concept.

Whether we have multiple overload, we use an essence pattern or
Boost.Parameter can be decided later. What we need are two concepts for
Arguments and Environment that can be implemented as efficiently as we would
do when using the platform specific interfaces. 

-- 
View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-Process-library-starts-tomorrow-tp3263607p3264839.html
Sent from the Boost - Dev mailing list archive at Nabble.com.