Formal Review of std.process

[Steven Schveighoffer]

On Fri, 12 Apr 2013 07:31:28 -0400, Lars T. Kyllingstad  
<public at kyllingen.net> wrote:

> On Thursday, 11 April 2013 at 15:43:18 UTC, Steven Schveighoffer wrote:
>> A couple minor comments:
>>
>> 1. I have two issues with Error being used.  One is that we should have  
>> a specific type that is thrown, not raw Error type.
>>  Second is that I think in situations where the error is due to an  
>> incorrect parameter, it should be an exception not an error (and not a  
>> straight Exception either!).
>
> Let's go through the places where an Error or Exception is thrown:
>
> spawnProcess() throws RangeError when args[] is empty, but this is just  
> the normal behaviour of arrays, and with -release/-noboundscheck it just  
> segfaults.  As such, there is little point in specifying this in the  
> documentation.  I'll remove it.  (Honestly, I don't know why I put it in  
> there in the first place.  It may have had something to do with me being  
> thoroughly annoyed over other the lack of exception specifications in  
> Phobos documentation at large.  I used to like enforce(), but now I  
> think it has given D programmers a way too lax attitude towards error  
> handling.)

Oh, this really needs clarification!  I was under the assumption that you  
are specifically checking this and throwing RangeError, even in release  
mode.  Please update the docs to reflect this.

I think it's worth noting in the docs that it will throw RangeError or  
segfault.  It may not be obvious that the function will not do this check  
for you in release mode.  I'd almost lean towards making the check even in  
release mode, but you can still throw RangeError.

>
> kill() throws Error if the code/signal is negative.  I suspect the cases  
> where this number comes directly from user input are so few and far  
> between that it is reasonable to expect the programmer to ensure that it  
> is nonnegative.  In principle, on POSIX we don't need the check, because  
> POSIX kill() will return an "invalid signal" error if you try to give it  
> a negative number.  On Windows, however, TerminateProcess() takes an  
> unsigned integer (which is why I added the check in the first place),  
> and I think we should strive to have the same behaviour on all platforms  
> to the extent possible.  I would not be strongly opposed to making this  
> an Exception of some kind, though, if you think there is a good reason  
> to do so.

Think of re-implementing command-line kill :)

In the end, an exception and error are quite the same, except for the fact  
that you SHOULDN'T catch errors.  The place where this makes a difference  
is whether the parameter comes from user input or not.  In the case of a  
kill signal, I can potentially see a user specifying a non-existent  
signal.  This should be caught and a nice error message explaining the  
usage or whatnot should be displayed.

> pipeProcess throws Error on an invalid combination of Redirect flags,  
> and ProcessPipes does the same on an attempt to access a non-redirected  
> stream.  Are we in agreement that these two are always programming  
> errors?

These two are separate.  I think the invalid combination of redirect flags  
is not beyond user-specification (think of processing 'cmd > out.txt 1>&2'  
)

Accessing a non-existent member is always a programming error, there is no  
recourse to that, error is fine here.

> escapeShellCommand() throws Error if the input contains \0, \r or \n.   
> Here, it is very likely that the arguments are user input, but it would  
> be very strange application code that somehow allowed those three  
> characters to enter the input.  Even so, it may be better to make it an  
> Exception, just to be safe.

Again, it's whether it's catchable or not.  Errors should only be caught  
by the runtime (IMO).

In order to do this correctly then (and print a nice error message for the  
user), you have to first verify the arguments before passing into the  
function, and not call the function.  Essentially you have to duplicate  
the checking code in the application, and then have the library run the  
same thing!

I think exception here as well.

-Steve