On 2013-04-12 13:31, Lars T. Kyllingstad wrote:

> I used to like enforce(), but now I
> think it has given D programmers a way too lax attitude towards error
> handling.)

What's wrong with "enforce" is that it's possible to not specify what kind of exception to thrown. Otherwise I kind of like it.

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

What do you thrown when POSIX kill() returns an "invalid signal" error?

-- 
/Jacob Carlborg

On 2013-04-12 13:05, Lars T. Kyllingstad wrote:

> Then let me ask you/them this:  Would you prefer it was implemented as a
> module-level @property function that returns a singleton class instance
> instead?  In other words, would you have the same API with slightly
> worse performance and more complicated implementation, *just* for the
> sake of a strict naming convention?
>
> Note that 'environment' (with that name) was added to std.process years
> ago.  As Jesse mentioned, the fact that it was a class was just hidden
> from documentation with an alias.  Unfortunately, this prevented proper
> documentation of its members, so changed it.

Ok, I see. I have don't have _that_ strong feelings about this. I also wouldn't have any problem of using it as class, like it is:

Environment["foo"] = "bar";

-- 
/Jacob Carlborg

On Fri, 12 Apr 2013 07:31:28 -0400, Lars T. Kyllingstad <public@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

The templated pipeProcessImpl only contains a single unshared line of code [1]. It might be worthwhile to reduce the amount of duplication by using delegates instead of alias parameters.

pipeProcess
{
    return pipeProcessImpl(
        (SpawnFuncArgs args) => spawnProcess(program, args),
        redirectFlags, env, config);
}

pipeShell
{
    return pipeProcessImpl(
        (SpawnFuncArgs args) => spawnShell(command, args),
        redirectFlags, env, config);
}

private alias TypeTuple!(File, File, File, const string[string], Config) SpawnFuncArgs;

private ProcessPipes pipeProcessImpl(scope Pid delegate(SpawnFuncArgs) spawnFunc,
                                     Redirect redirectFlags,
                                     const string[string] env = null,
                                     Config config = Config.none)
{
    // ... shared code
    auto pid = spawnFunc(childStdin, childStdout, childStderr, env, config);
}

[1]: https://github.com/kyllingstad/phobos/blob/994437169f20907f30515cfef54538710f3fc1fe/std/process.d#L1636

On Sat, 13 Apr 2013 16:16:09 -0400, Martin Nowak <code@dawg.eu> wrote:

> The templated pipeProcessImpl only contains a single unshared line of code [1]. It might be worthwhile to reduce the amount of duplication by using delegates instead of alias parameters.

I think it's a valid point, but I also think the duplication is not that bad at the moment, there are only two instantiations.  However, if we ever got to the point where we were templating on the command string type, it would be more savings by using a delegate.  This optimization is also fully internal, so it can be submitted as an enhancement after acceptance.

-Steve

04-Apr-2013 18:22, Jesse Phillips пишет:
> Hello,
>
> I have decided to take on Review Manager for std.process and hopefully
> will keep it up and get through the review queue. This is slightly
> ignoring a "first come, first serve" approach, but that is mostly
> because I didn't want to look into it. (So don't be too upset).
>
> std.process has been under review for some time having discussion on the
> pull request and this thread:
> http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue@forum.dlang.org

Getting late on this one.

What is a plan (if any) to migrate towards something else then File for streams once we have it (e.g. std.io by Steve)?

-- 
Dmitry Olshansky

On Tue, 16 Apr 2013 05:23:32 -0400, Dmitry Olshansky <dmitry.olsh@gmail.com> wrote:

> 04-Apr-2013 18:22, Jesse Phillips пишет:
>> Hello,
>>
>> I have decided to take on Review Manager for std.process and hopefully
>> will keep it up and get through the review queue. This is slightly
>> ignoring a "first come, first serve" approach, but that is mostly
>> because I didn't want to look into it. (So don't be too upset).
>>
>> std.process has been under review for some time having discussion on the
>> pull request and this thread:
>> http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue@forum.dlang.org
>
> Getting late on this one.
>
> What is a plan (if any) to migrate towards something else then File for streams once we have it (e.g. std.io by Steve)?

My library will provide a replacement for the back-end of File (i.e. FILE *), so it will just work.

-Steve