On Fri, 12 Apr 2013 07:31:28 -0400, Lars T. Kyllingstad
<[email protected]> 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
