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-tt... 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-P... Sent from the Boost - Dev mailing list archive at Nabble.com.