Re: [boost] Presentation of Boost.Process (SoC project)

From: Oleg Abrosimov (beholder_at_[hidden])
Date: 2006-08-17 13:13:58

Hello Julio,

First of all, I want to thank you and your mentor for the new great 
library. I've started to read it's docs from top to bottom and I have a 
few comments till now:

1) The motivation part is too long. Process handling is a hot topic for 
most of us and there is no need to prove that your library is great and 
very useful. It was important when you made your proposal for SoC, but 
now your goal is to provide a very short start for your reader and 
potential user of your library. (Sorry for so long paragraph, hope it 
would be useful)

2) why you don't abstract out things like EXIT_FAILURE/EXIT_SUCCESS? 
they are artifacts from C and looks ugly in C++ code. simple enum would 
be just enough.

3) seems that you need to fix you quickbook code:
// quickbook:begin(command-line)
bp::command_line cl("svn");
cl.argument("update");
// quickbook:end(command-line)

4) command_line constructor is defined as
command_line(const std::string & executable,
              const std::string & firstarg = "",
              const std::string & path = "");

docs says that
executable      The full path to the executable program.
firstarg        The program's name. If empty, the constructor sets it to 
be the executable's base name (i.e., skips the directory).
path            Set of directories where to look for the executable, if 
it does not include any path component.

the difference of first and second arguments are not clear and purpose 
of the second argument too. command_line class provides a member 
function called argument, so the name firstarg suggests that it is the 
first argument passed to the executable under question. Resolve this 
confusion, please.

5) the argument member function is non-intuitive. it is a noun but 
should be a verb like add_argument. The name proposed is even more 
verbose than current one, it leads to the next suggestion: use 
'operator<<' to append arguments to command line object.

6) const arguments_vector & get_arguments(void ) const;
is it really needed? in what scenario other then dump for debugging 
purposes?
what an invariant it tries to maintain? if there is no one, then it can 
be simply
public:
  arguments_vector arguments;

the one argument for member function against simple member is to ensure 
immutability (the immutable object pattern) but it is not true for 
arguments. they can be added. (but not removed oops!)

It seems that command_line tries to be both, path to executable and 
arguments to it. first can be made immutable and it can be proved to be 
a good thing (or can it?) but arguments are different. one needs to pass 
different arguments for the same executable in a case when it fails for 
previous attempt for example. May be it is worth to factor it out of 
command line and make command_line immutable, but arguments - mutable?

bp::command_line cl("svn");
cl.argument("update");

becomes

fs::path exec("svn");
bp::command_line_arguments args("update"); // supports up to N 
parameters to be passed in constructor + modifier/composition functions 
for reuse
bp::command_line cl(exec, args);

7) const std::string & get_executable(void ) const;
same questions are applied here + one more - did you considered to 
return boost::filesystem::path here? if yes, why it was rejected?
+ I believe that get_executable_path() would better reflect its purpose.
(Note, I would prefer executable_path public member)

8) static command_line shell(const std::string & command) ;
from docs to this function I've realized that this function can not be 
used in portable way. The question is - why it is in a portable part of 
the library? it can be replaced by a pair of free factory functions in 
corresponding platform specific namespaces, like:
command_line posix_shell_command(const std::string & command, const 
std::string & shell_name)

shell_name allows to pass command not only for sh, but any other 
platform specific shell, like python, for example.

9) the launcher. It seems to be just a container for different process' 
attributes. (It's documentation shows only one function start. fix your 
quickbook please) Same invariant issues applies here too. it has no 
invariant at all and can be a simple struct with all members are public 
and simply assignable without setter methods.

bp::launcher l;
l.set_stdout_behavior(bp::redirect_stream);
l.set_merge_out_err(true);
l.set_work_directory(dir);

would be

bp::launcher l;
l.stdout_behavior = bp::redirect_stream;
l.merge_out_err = true;
l.work_directory = dir; //should be a fs::path object ?

Here again two concepts are coupled:
a collection of attributes or context and the action of launching the 
child process. if we decouple it we get the following:

bp::context cntx;
cntx.stdout_behavior = bp::redirect_stream;
cntx.merge_out_err = true;
cntx.work_directory = dir; //should be a fs::path object ?

bp::child c = bp::launch(cntx, cl);

// was: bp::child c = l.start(cl); - the start member function looks 
like a strange artifact.

10) all relevant pieces should be parameterized against char_type and 
traits_type

11) docs says that child' constructor is protected, but in synopsis it 
is under public section...

12) the following methods adds nothing over exposing underlying streams 
directly (overincapsulated again):
   postream & get_stdin(void) const;
   pistream & get_stdout(void) const;
   pistream & get_stderr(void) const;

+ they are strange because they are const, but returns non-const reference.

13) description for status class says:
"This class represents the status returned by a child process after it 
has terminated. It contains many methods that may make no sense in some 
platforms, but this is done for simplicity. These methods are supported 
everywhere, although they may simply return fake values"

but it smells not very good. immediate question arises - do there 
alternatives exists? enumerate them and prove that the current solution 
is better.

I propose to make two platform-specific status classes say, posix_status 
and win32_status and their (set) intersection - status

then if one doesn't care about platform specific issues he writes:
bp::status s = c.wait();

and if one is care about it, he writes:
bp::posix_status s = c.wait(); //posix_status' constructor accepts 
status object. it is a simplified form of polimorphism.

14) constructor status(int flags); is public. Why? Am I (as a user) 
allowed to construct it directly?

15) if (s.exited() && s.exit_status() == EXIT_SUCCESS)
looks not so good ;-) see the issue (2).
I would prefer
if (s.exited() && s == status::exit_success)

or

if (s.exited() && s == bp::exit_success)

where exit_success is a const status object or object of any other 
special type (to avoid unsafe comparisons to int in case of using enum)

16) one final comment: status object is already immutable. it can be 
enforced if child::wait() would returns const status object.
after this you can eliminate most of member functions and replace em 
with plain members. it would simplify its usage.

It is already too much for one letter. I'm stop here.
Hope that my comments would help you to improve the library.

Best,
Oleg Abrosimov.