Subject: Formal Review of Proposed Boost.Process library starts tomorrow

classic Classic list List threaded Threaded
91 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Subject: Formal Review of Proposed Boost.Process library starts tomorrow

Marshall Clow-2
It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs.

What is it?
===========

Boost.Process is a library to manage system processes. It can be used to:

        • create child processes
        • run shell commands
        • setup environment variables for child processes
        • setup standard streams for child processes (and other streams on POSIX platforms)
        • communicate with child processes through standard streams (synchronously or asynchronously)
        • wait for processes to exit (synchronously or asynchronously)
        • terminate processes


Getting the library
===================

The library is available at:
        http://www.highscore.de/boost/gsoc2010/process.zip
with documentation at:
        http://www.highscore.de/boost/gsoc2010/

There was a quite involved "informal review" on the mailing list this past summer.
That thread is archived here:
        http://thread.gmane.org/gmane.comp.lib.boost.devel/207594

Writing a review
================

If you feel this is an interesting library, then please submit your
review to the developer list (preferably), or to the review manager (me!)

Here are some questions you might want to answer in your review:

- What is your evaluation of the design?
- What is your evaluation of the implementation?
- What is your evaluation of the documentation?
- What is your evaluation of the potential usefulness of the library?
- Did you try to use the library? With what compiler? Did you have
any problems?
- How much effort did you put into your evaluation? A glance? A quick
- reading? In-depth study?
- Are you knowledgeable about the problem domain?

And finally, every review should answer this question:

- Do you think the library should be accepted as a Boost library?

Be sure to say this explicitly so that your other comments don't
obscure your overall opinion.

-- Marshall
Review Manager for the proposed Boost.Process library

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Oliver Kowalke-2
I did a brief look into the docu and the "informal review" thread.

I would suggest to provide a function indicating if you are running on POSIX or Windows platform:

class process {
...
static bool is_posix();
...
}

The lib could provide an encapsulation for testing the exit code against
WIFEXITED, WEXITSTATUS, WIFSIGNALED, WTERMSIG, WIFSTOPPED, WSTOPSIG, WIFCONTINUED, WCOREDUMP (as the old version of boost.process did).

Sending signals (SIGSTOP, SIGCONT, SIGKILL, etc.) to the process would also be possible.

In the case this functionality is used on Windows paltform without testing process::is_posix() you could raise an assertion.


Oliver
--
Neu: GMX De-Mail - Einfach wie E-Mail, sicher wie ein Brief!  
Jetzt De-Mail-Adresse reservieren: http://portal.gmx.net/de/go/demail
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
In reply to this post by Marshall Clow-2
AMDG

Review part 1, documentation:

* Introduction: eg. should be e.g.

* create_child:
   since the argument is the path to the file, why
   don't you take a boost::path, instead of a string?

* It seems like redirecting a stream to a file should
   be supported.  Maybe it can be done with inherit,
   but it isn't immediately obvious and I'd rather
   not deal with handle::native_type at all.

* There are pistream and postream.  What about
   piostream for completeness.

* Why do you have to pass a native handle to
   pipe's constructor.  Can't it be overloaded
   to take a handle?

* "On POSIX systems you must use the macros defined
   in sys/wait.h to interpret exit codes."

   This forces me to use #ifdefs in my code.  Isn't
   there a way to abstract this better?  At least
   you could wrap the macros and have a trivial
   implementation for windows (WIFEXITED -> true)

* context::setup: The reference specifies boost::function< void()>,
   but this is obviously only true for posix.  It should
   describe the two different behaviors.

* std::string context::work_dir; boost::path?

* boost::iostreams has a file_descriptor class
   that is very similar to handle.

* What is the behavior of find_executable_in_path W.R.T.
   extensions.  On windows, does it use PATHEXT?
   I'd like to see this spelled out explicitly.

* What about file associations/#!xxx for create_child?
   i.e. can I run create_child("script.pl") or do I need
   to use shell?  I can live with it either way, but
   the reference needs to specify it.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
AMDG

On 2/7/2011 10:22 AM, Steven Watanabe wrote:
> * context::setup: The reference specifies boost::function< void()>,
> but this is obviously only true for posix. It should
> describe the two different behaviors.
>

I see that the doxygen comments do have this.
It looks like a BoostBook problem.  I'll
see what I can do about it.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
In reply to this post by Marshall Clow-2
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.
Reply | Threaded
Open this post in threaded view
|

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

Oliver Kowalke-2
In reply to this post by Marshall Clow-2
I would suggest to provide a function indicating if you are running on a
POSIX platform:

bool is_posix_platform();

if ( boost::process::is_posix_platform() ) {
    ...
}

The lib could provide an encapsulation for testing the exit code against
WIFEXITED, WEXITSTATUS, WIFSIGNALED, WTERMSIG, WIFSTOPPED, WSTOPSIG,
WIFCONTINUED, WCOREDUMP (as the old version of boost.process did).

posix_status ps( status);
if ( ps.terminated() ) int sig = ps.termination_signal();
if ( ps.has_coredump() ) ...

Sending signals (SIGSTOP, SIGCONT, SIGKILL, etc.) to the process would
also be usefull.

You could raise an assertion (BOOST_ASSERT) if this functionality will
be invoked by a non-POSIX platform.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Mathias Gaunard-2
In reply to this post by Steven Watanabe-4
On 07/02/2011 19:22, Steven Watanabe wrote:

> * There are pistream and postream. What about
> piostream for completeness.

Pipes are either read-only or write-only; there could however be a
mechanism to aggregate two pipes into a single iostream.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Christopher Jefferson
In reply to this post by Marshall Clow-2
This is only a short review.

I installed boost.process on ubuntu and ran some of the examples. The library appears well documented and works correctly.

I have previously used a process library I created myself. Boost process has much more functionality than my library, and I will use it if it accepted into boost.

I would prefer it if the library had some simple helpers for simple uses. In particular I would like an example which runs a simple program like "ls" and captures all output and the return value.

In conclusion, I would believe from my brief review boost.process should be accepted. I would like to see more examples showing simple uses, and if these examples turn out to be more than a couple of lines, simple wrapper functions which help users with simple use cases.

Chris

On 7 Feb 2011, at 06:18, Marshall Clow wrote:

> It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs.
>
> What is it?
> ===========
>
> Boost.Process is a library to manage system processes. It can be used to:
>
> • create child processes
> • run shell commands
> • setup environment variables for child processes
> • setup standard streams for child processes (and other streams on POSIX platforms)
> • communicate with child processes through standard streams (synchronously or asynchronously)
> • wait for processes to exit (synchronously or asynchronously)
> • terminate processes
>
>
> Getting the library
> ===================
>
> The library is available at:
> http://www.highscore.de/boost/gsoc2010/process.zip
> with documentation at:
> http://www.highscore.de/boost/gsoc2010/
>
> There was a quite involved "informal review" on the mailing list this past summer.
> That thread is archived here:
> http://thread.gmane.org/gmane.comp.lib.boost.devel/207594
>
> Writing a review
> ================
>
> If you feel this is an interesting library, then please submit your
> review to the developer list (preferably), or to the review manager (me!)
>
> Here are some questions you might want to answer in your review:
>
> - What is your evaluation of the design?
> - What is your evaluation of the implementation?
> - What is your evaluation of the documentation?
> - What is your evaluation of the potential usefulness of the library?
> - Did you try to use the library? With what compiler? Did you have
> any problems?
> - How much effort did you put into your evaluation? A glance? A quick
> - reading? In-depth study?
> - Are you knowledgeable about the problem domain?
>
> And finally, every review should answer this question:
>
> - Do you think the library should be accepted as a Boost library?
>
> Be sure to say this explicitly so that your other comments don't
> obscure your overall opinion.
>
> -- Marshall
> Review Manager for the proposed Boost.Process library
>
> _______________________________________________
> Boost-users mailing list
> [hidden email]
> http://lists.boost.org/mailman/listinfo.cgi/boost-users

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
In reply to this post by Steven Watanabe-4
AMDG

Review part 2: implementation

boost/process.hpp:

boost/process/all.hpp:

boost/process/child.hpp:

The PP guard on the constructor that takes
a handle, should be
#if defined(BOOST_WINDOWS_API) || defined(BOOST_PROCESS_DOXYGEN)
so that it appears in the reference.

The map should be passed by const std::map<stream_id, handle>&
(No need to make extraneous copies.)

windows.h is not needed.

boost/process/config.hpp:

Can you use BOOST_THROW_EXCEPTION instead
of rolling your own file/line info?

boost/process/context.hpp:

boost/process/environment.hpp:

boost/process/handle.hpp:

The semantics of handle::dont_close appear to
be the same as the original boost::iostreams::file_descriptor,
in that an explicit call to close still
closes the handle.  This led to some problems,
so Daniel James changed it to have three
options.  See ticket #3517.  I'm wondering
whether similar problems are liable to
come up here.

If the handle is supposed to be closed and
construction fails, the native handle should
be closed.  Otherwise, it's difficult make
code using it exception safe.

boost/process/operations.hpp:

find_executable_in_path:

I'm not sure that asserting is the right response
when the name contains a "/."  It's not unreasonable
to expect that the name can come from outside the
program, and thus may not be valid.  If you do go
with assert, the precondition needs to be documented.

On Windows, it's possible to have a path longer than
MAX_PATH.  You should probably use the size returned
from SearchPathA to allocate a buffer that's big enough
and try again.

Cygwin executables end with .exe, just like windows.

create_child:
The context should be passed by const reference.

The requirements on Context and Arguments need to
be documented.

Is not exception safe with BOOST_POSIX_API because
if environment_to_envp throws, the memory allocated
by collection_to_argv will not be released.

I'd like everything after the child process is
started to be no-throw, so I can guarantee that
if the child is created, I can get a handle to it.
This includes returning the child object.

boost/process/pid_type.hpp:

boost/process/pipe.hpp:

Can you #include the appropriate specific
header, rather than the whole boost/asio.hpp?

boost/process/pistream.hpp:

boost/process/postream.hpp:

boost/process/process.hpp:

boost/process/self.hpp:

boost/process/status.hpp:

boost/process/stream_behavior.hpp:

boost/process/stream_ends.hpp:

boost/process/stream_id.hpp:

boost/process/stream_type.hpp:

boost/process/detail/basic_status.hpp:

Again, #include <boost/asio.hpp> is a lot more than you need.

boost/process/detail/basic_status_service.hpp:

The constructor is not exception safe.  If
starting the thread fails, the event will
be leaked.

work_thread can throw which will terminate
the process.  Is this the desired behavior
or should it be using Boost.Exception to
move the exception to a different thread?

async_wait can leak the HANDLE created
with OpenProcess.

condition variables are allowed to have
spurious wakeups.  You need to track
enough state to make sure that you don't
wake up early.

boost/process/detail/posix_helpers.hpp:

environment_to_envp and environment_to_envp
are not exception safe.

boost/process/detail/status_impl.hpp:

operation: If an argument is not used,
virtual void operator()(int /* exit_code */)
is the best way to suppress warnings.

boost/process/detail/systembuf.hpp:

I don't think that overflow is correct.
According to the MSDN documentation WriteFile
can return when:

     * The number of bytes requested is written.
     * A read operation releases buffer space on the read
       end of the pipe (if the write was blocked). For
       more information, see the Pipes section.
     * An asynchronous handle is being used and the write
       is occurring asynchronously.
     * An error occurs.

Note #2 in particular.

boost/process/detail/windows_helpers.hpp:

Can you #include <cstring> instead of <string.h>?

General:

The #include <windows/unistd.h> is
not consistent.  Sometimes there's
an extra

#else
#   error "Unsupported platform."
#endif

sometimes not.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
In reply to this post by Marshall Clow-2
Hi,

I want to discuss here of how transparent would be the user code to be able to use non portable extension with the current interface. The way extensions are provided means that a user needs to use the BOOST_POSIX_API, BOOST_WINDOWS_API in his own code when he uses an extension. Could the documentation include some explicitly examples.

The interface of the setup callback is different so the user will need to define two possible setup functions.

#if defined(BOOST_POSIX_API)
void setup() {
 ...
}
#elif defined(BOOST_WINDOWS_API)
void setup(STARTUPINFOA &sainfo) {
 ...
}
#endif

Note that these setup functions are called on a different context either on the child either on the parent process, but both will be assigned to the same context field.

Any setting of the ctx.streams with fd >2 should be protected

ctx.streams[3] = ...

While the preceding code doesn't have any sens under WINDOWS, the library don't provide an interface that forbids it, but the current implementation just ignore it. I'm not sure if this is good in this case as the application could not work in the same way in POSIX or WINDOWS systems. If the class stream_id I proposed in this thread is used as domain of the map, the code will not compile as a stream_id cannot be constructed from fd>2 on WINDOWS.

The opposite applies to behaviors whose constructor have stream_type parameter. These constructors are available only for POSIX, but I don't think adding them will change the perception of the user has of his application. Couldn't the behaviors provide a uniform interface for these constructors? BTW all the constructor taking a stream_type as parameter are not documented.

Instead of just defining them for POSIX

#if defined(BOOST_POSIX_API)
    pipe()  : stype_(unknown_stream)  { }
    pipe(stream_type stype) : stype_(stype)  { }
#endif

You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.

#if defined(BOOST_WINDOWS_API)
    pipe() { }
    pipe(stream_type /*stype*/)  {    }
#endif

The same applies to all the behaviors using stream_type.

Could a table of extension be included on the documentation?

Best,
Vicente
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
In reply to this post by Vicente Botet
AMDG

On 2/7/2011 11:09 AM, Vicente Botet wrote:

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

This isn't possible with the Doxygen/BoostBook
toolchain.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
Steven Watanabe-4 wrote
AMDG

On 2/7/2011 11:09 AM, Vicente Botet wrote:
> 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.
>

This isn't possible with the Doxygen/BoostBook
toolchain.
Hrrr. It's a shame :(

Well I will then be happy with a table compiling the differences.

Vicente
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
In reply to this post by Vicente Botet
AMDG

On 2/7/2011 11:09 AM, Vicente Botet wrote:
>
> private functions like invalid_handle don't need to be documented. Shouldn't
> the nested impl class be private?
>

impl is private.  I usually wrap private members
in \cond \endcond to avoid this.  It should probably
be fixed in the doxygen to boostbook conversion, however.

> [pistream/postream]
> class pistream : public std::istream, public boost::noncopyable
>
> * inheritance missing on the documentation.

It's lost by doxygen.  I don't know whether there's an
easy way to fix it.

> * constructor must be documented explicit
>

Fixed in trunk BoostBook.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
Steven Watanabe-4 wrote
AMDG

On 2/7/2011 11:09 AM, Vicente Botet wrote:
>
> private functions like invalid_handle don't need to be documented. Shouldn't
> the nested impl class be private?
>

impl is private.  I usually wrap private members
in \cond \endcond to avoid this.  It should probably
be fixed in the doxygen to boostbook conversion, however.
There are no \cond \endcond on the code. Doesn't Doxigen have a generation flag to state which level is documented?


> [pistream/postream]
> class pistream : public std::istream, public boost::noncopyable
>
> * inheritance missing on the documentation.

It's lost by doxygen.  I don't know whether there's an
easy way to fix it.


Maybe we need to include a specific forward declaration file while building the documentation with Doxigen?


> * constructor must be documented explicit
>

Fixed in trunk BoostBook.



It would be great if we had a wiki page on which we state explicitly all the limitation and good usage of Doxigen.

Thanks,
Vicente
Reply | Threaded
Open this post in threaded view
|

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

Ilya Sokolov-2
In reply to this post by Vicente Botet
On Tue, 08 Feb 2011 03:26:57 +0500, Vicente Botet  
<[hidden email]> wrote:

>
> Any setting of the ctx.streams with fd >2 should be protected
>
> ctx.streams[3] = ...
>
> While the preceding code doesn't have any sens under WINDOWS, the library
> don't provide an interface that forbids it, but the current  
> implementation
> just ignore it. I'm not sure if this is good in this case as the  
> application
> could not work in the same way in POSIX or WINDOWS systems. If the class
> stream_id I proposed in this thread is used as domain of the map, the  
> code
> will not compile as a stream_id cannot be constructed from fd>2 on  
> WINDOWS.

I think it is enough to assert on valid stream_ids in the create_child()  
function.

> The opposite applies to behaviors whose constructor have stream_type
> parameter. These constructors are available only for POSIX, but I don't
> think adding them will change the perception of the user has of his
> application. Couldn't the behaviors provide a uniform interface for these
> constructors? BTW all the constructor taking a stream_type as parameter  
> are
> not documented.
>
> Instead of just defining them for POSIX
>
> #if defined(BOOST_POSIX_API)
>     pipe()  : stype_(unknown_stream)  { }
>     pipe(stream_type stype) : stype_(stype)  { }
> #endif
>
> You can add the respective constructors for WINDOWS, even if the  
> parameter
> is completely ignored.
>
> #if defined(BOOST_WINDOWS_API)
>     pipe() { }
>     pipe(stream_type /*stype*/)  {    }
> #endif
>
> The same applies to all the behaviors using stream_type.

Those constructors have a sense on windows, no need to ignore anything.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Ilya Sokolov-2
In reply to this post by Steven Watanabe-4
On Mon, 07 Feb 2011 23:22:24 +0500, Steven Watanabe <[hidden email]>  
wrote:

> AMDG
>
> Review part 1, documentation:
>
> * Introduction: eg. should be e.g.
>
> * create_child:
>    since the argument is the path to the file, why
>    don't you take a boost::path, instead of a string?

Then what type to choose for context::env?

I stated my opinion on the i18n problem earlier:
http://article.gmane.org/gmane.comp.lib.boost.devel/207745

> * It seems like redirecting a stream to a file should
>    be supported.  Maybe it can be done with inherit,
>    but it isn't immediately obvious and I'd rather
>    not deal with handle::native_type at all.

+1

> [snip]
>
> * Why do you have to pass a native handle to
>    pipe's constructor.  Can't it be overloaded
>    to take a handle?

I'm not sure which pipe do you have in mind. Probably that means
that the names bp::pipe, bp::stream_behavior and bp::stream_ends
are not perfect.

> [snip]
>
> * std::string context::work_dir; boost::path?

see above

> [snip]

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
In reply to this post by Steven Watanabe-4
<quote author="Steven Watanabe-4">
AMDG

Review part 1, documentation:

* Introduction: eg. should be e.g.

* "On POSIX systems you must use the macros defined
   in sys/wait.h to interpret exit codes."

   This forces me to use #ifdefs in my code.  Isn't
   there a way to abstract this better?  At least
   you could wrap the macros and have a trivial
   implementation for windows (WIFEXITED -> true)
+1

* context::setup: The reference specifies boost::function< void()>,
   but this is obviously only true for posix.  It should
   describe the two different behaviors.
Is this yet a limitation of Doxigen?

Vicente
Reply | Threaded
Open this post in threaded view
|

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

Steven Watanabe-4
AMDG

On 2/7/2011 11:06 PM, Vicente Botet wrote:
>>
>> * context::setup: The reference specifies boost::function<  void()>,
>>     but this is obviously only true for posix.  It should
>>     describe the two different behaviors.
>>
>
> Is this yet a limitation of Doxigen?
>

The problem is in our stylesheets.  I posted
a patch to the doc list earlier today.

In Christ,
Steven Watanabe
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Ilya Sokolov-2
In reply to this post by Steven Watanabe-4
On Tue, 08 Feb 2011 02:06:09 +0500, Steven Watanabe <[hidden email]>  
wrote:

> AMDG
>
> Review part 2: implementation
>
> [snip]
>
> boost/process/config.hpp:
>
> Can you use BOOST_THROW_EXCEPTION instead
> of rolling your own file/line info?

I think BOOST_PROCESS_SOURCE_LOCATION is my piece of code,
which was written because BOOST_THROW_EXCEPTION didn't work
(IIRC, there was a problem with BOOST_CURRENT_FUNCTION)

> [snip]

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Denis Shevchenko
In reply to this post by Vicente Botet
My 2 cents, about source code...

1. In class boost::process::child.

      There is a type 'std::map<stream_id, handle>' that used several
times. May be typedef?

2. In config.hpp file.

     Macros will be easier for maintenance if align backlslashes.
     Compare:

     #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \
     boost::throw_exception(boost::system::system_error( \
         boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \
             boost::system::get_system_category()), \
         BOOST_PROCESS_SOURCE_LOCATION what))

     and

     #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what)         \
     boost::throw_exception(boost::system::system_error(
                         \
         boost::system::error_code(BOOST_PROCESS_LAST_ERROR,            \
             boost::system::get_system_category()),                    
                   \
         BOOST_PROCESS_SOURCE_LOCATION what))

3. In constructor of class boost::process::context.

     In this place you insert a values into map, but not search them.
     So use insert() instead of operator[]. Of course, with
boost::assign ;-)

     using namespace boost::assign;
     insert( streams )( stdin_id,    behavior::inherit(STDIN_FILENO) )
                               ( stdout_id,
behavior::inherit(STDOUT_FILENO) )
                               ( stderr_id,  
behavior::inherit(STDERR_FILENO) )
                               ;

4. In some classes you use public inheritance from boost::noncopyable,
but you can use private inheritance instead.

5. In constructors of some classes you forgot 'explicit'.

6. In function boost::process::self::get_environment().

         environment e;
          // ...
         try
         {
             char *env = ms_environ;
             while (*env)
             {
                 std::string s = env;
                 std::string::size_type pos = s.find('=');
                 e.insert(environment::value_type(s.substr(0, pos),
                     s.substr(pos + 1)));
                 env += s.size() + 1;
             }
         }
         catch (...)
         {
             // ...
         }

         And what kind of exceptions can occur here?
             std::string::find,
             std::string::substr,
             std::map::insert,
             std::string::operator+=
             std::string::size
         these functions do not throw exceptions...

7. In class boost::process::detail::basic_status_service.

     There is a type 'boost::unique_lock<boost::mutex>' that used many
times. May be typedef?

8. In some places of code you using std::algorithms. You can use
boost::range::algorithms instead, for simplicity.

     Compare:

     std::find(impls_.begin(), impls_.end(), impl);

     and:

     boost::range::find( impls_, impl );

9. You can replace some 'native' cycles by BOOST_FOREACH.

10. You can initialize map easier:

     Compare:

     std::map<stream_id, handle> parent_ends;
     parent_ends[stdin_id] = handles[stdin_id].parent;
     parent_ends[stdout_id] = handles[stdout_id].parent;
     parent_ends[stderr_id] = handles[stderr_id].parent;

     and:

     using namespace boost::assign;
     std::map<stream_id, handle> parent_ends = map_list_of( stdin_id,  
handles[stdin_id].parent )
                                                          ( stdout_id,
handles[stdout_id].parent )
                                                          ( stderr_id,
handles[stderr_id].parent )
                                                          ;

11. In many places of code you using std::map. May be better to use
boost::unordered_map, for optimizing the speed of searching?

Best regards, Denis.
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
12345