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

From: Oleg Abrosimov (beholder_at_[hidden])
Date: 2006-08-23 00:28:29

Julio M. Merino Vidal wrote:
>> 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)
> 
> What do you mean?  I can't see the problem there.

this code snippet is from the library docs. such a comment adds nothing 
here (it can not improve understanding of code), furthermore, it 
clutters code and makes it less readable. That's why I've concluded that 
it is a quickbook misconfiguration.

>> 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.
> 
> Agreed.  I'd rather call it 'add'.  And maybe also provide operator<<.
>  Could the operator make things clearer?
> 
> cl.add("foo").add("bar").add(4);
> cl << "foo" << "bar" << 4;
> 
>> 6) const arguments_vector & get_arguments(void ) const;
>> is it really needed? in what scenario other then dump for debugging
>> purposes?

Now I've realized that this stuff is not needed at all. It is used to 
construct the string that you pass to function shell() (I've simplified 
things a bit). Alternatively, you could remove arguments stuff and 
provide only the shell() function and leave a user with constructing the 
arguments string by itself. The reason is that if you are going to 
provide a way to simplify the process - you are trying to do too much. 
It is not an easy task to create a sub-library to manipulate program 
arguments that would satisfy the majority of us. For example, what about 
not only adding but also removing arguments and maybe replacing? To 
conclude - it is a separate task to create an arguments manipulating 
sub-library. You should better don't do it now. May be later...

> 
> Hmm... separating the two concepts sounds good.  Maybe the executable
> can be completely separated from the command_line, without adding a
> command_line_arguments class.  After all, the command line is an
> entity on its own that has nothing to do with the binary.  I.e.:

Sounds reasonable.

>> 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)
> 
> Don't know how yet.  launcher inherits from the detail::launcher_base
> class, which is documented in the source code but not in the generate
> documents.  Ideally, the launcher documentation in the manual could
> simply inherit that from its parent...
> 
> Somewhat related to this, maybe it'd also be worth to make
> launcher_base a public class.  It can be useful to people implementing
> a custom launcher.  Not that matters any more if launcher becomes
> 'context' as you suggest below.
> 
>> 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.
> 
> The current getters check for several sanity conditions (currently
> using assertions, but exceptions should be considered).  Moving to
> plain attributes means that these cannot take place any more :(

it suggests that you have an invariant here and your checks ensure it.
It can be a significant improvement if this hidden invariant becomes 
visible and explicit in user code. in this case you'll have no need to 
do any sanity checks (it is not always possible, though). For example, 
if object obj can be used only after function foo was called you can 
enforce it by returning the obj from foo. and prohibit any other way to 
obtain an obj.

>> 10) all relevant pieces should be parameterized against char_type and
>> traits_type
> 
> "Relevant pieces" refers to any class including/dealing with strings?
yes, strings and streams too

>> 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;
> 
> Again, these do sanity checks like e.g. not allowing you to get a
> reference to stdin if it was not redirected by the launcher.  (Yes,
> assertions could become exceptions here.)

alternatively, you can return pointer(or boost::optional) here

> 
> I'm assuming that you'd store all the status on the 'status' class,
> and its children could only provide getters for additional
> information, right?  If so, sounds better than the template approach
> because it's easier to use.
> 
> The only "strange" situation that can arise is this:
> 
> bp::status s = c.wait();
> 
> if (s.exited())
>    // OK, deal with exit status.
> else
>    // Uh... something happened but we cannot know exactly what.  Why?
> 
> (I'm only afraid of people trying to use exit_status() when exited()
> returns false in this specific situation because they'd see that
> exited() never returns true under Windows...)

The idea was that bp::status has all the information, but exposes 
publicly only the portable part. in order to obtain platform-specific 
info one needs to construct the platform specific status object and 
access it's member functions. For example, because status::exited() is 
not useful under win32 bp::status wouldn't contain this method.

> 
>> 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)
> 
> Indeed.  See reply to 2 ;-)

boost::tribool can be used to eliminate exited() function and all ugly 
comparisons with EXIT_SUCCESS/EXIT_FAILURE

from tribool docs:

// construction
tribool b(true);
b = false;
b = indeterminate;

// usage
tribool b = some_operation();
if (b) {
   // b is true
}
else if (!b) {
   // b is false
}
else {
   // b is indeterminate
}

> 
>> 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.
> 
> See status' code.  Again, getters do some sanity checks on the preconditions...

consider to enforce them in explicit fasion, if possible

Best,
Oleg Abrosimov