Fix Issue 11308 - Don't use Voldemort types for std.process output

[monarchdodra]

So that the return type of execute and executeShell are compatible.

Long story short, because the structure was declared in the template, it created two different types. This prevented the use of a single ret object:

auto ret = execute(...); //First declaration
ret = execute(...); //Ok
ret = executeShell(...); //Nope

So this "unlocks" that.

@schveiguy , I think you can review this?

[schveiguy]

Yes, this is a no brainer. If you used a different string width for the command line, it also would prevent reassignment.

Not sure it needs to be made private. But it was a voldemort type originally, @kyllingstad ?.

[kyllingstad]

Both functions are actually supposed to return std.typecons.Tuple!(int, "status", string, "output"), cf. the "Returns" section of their documentation. I think we should check whether the compiler bug that prevented this (I've forgotten the number, if it was ever reported) is still present, and make that change if possible. Otherwise, I'm all for merging this.

[ghost]

This is a bug I've filed already: https://issues.dlang.org/show_bug.cgi?id=11308

[monarchdodra]

Both functions are actually supposed to return std.typecons.Tuple!(int, "status", string, "output"), cf. the "Returns" section of their documentation.

What is the motivation for wanting to return a tuple? Seems just as convenient to return a named type to me.

[monarchdodra]

This is a bug I've filed already

Awesome. I've re-based and tagged my commit/pull accordingly.

[kyllingstad]

What is the motivation for wanting to return a tuple?

• It is a small and simple POD struct.
• It is used in only one (ok, two, but they're basically the same) place, and then only as a return value.
• It means one less symbol cluttering the namespace.

Seems like the prototypical use case for a Tuple to me.

[monarchdodra]

It is a small and simple POD struct.

Hum... I thought they weren't POD's, because they have elaborate assigns and constructors. But apparently, the elaborate assigns doesn't get in the way.

and then only as a return value.

Yeah, that's probably a good argument.

I'm fixing it now. EDIT: In a couple hours :)

[ghost]

I think right from the get-go we went a bit overboard with hiding everything under Voldemort/private structure types. It's useful for hiding things that have an API, e.g. ranges, but in this case I think it's overkill. It's essentially forcing people to use auto, even if some people like typing a variable explicitly, or e.g. want to store the structure to a field (they'd have to use typeof()/ReturnType tricks and whatnot).

[kyllingstad]

I completely agree. There are several times when I've been thoroughly annoyed that I couldn't easily store a voldemort type in a struct field without resorting to ugly tricks. However, in this case, the reason was to avoid breaking client code when we made the change to Tuple. When we make the change now, existing code will be source, and maybe even binary, compatible.

[kyllingstad]

I'm fixing it now.

Awesome, thanks! :-)

[kyllingstad]

I'm fixing it now.

Btw, I think both execute() and executeShell() should explicitly have Tuple as their signature return type, and not auto.

[monarchdodra]

Allright. I've now changed it (and tryWait) to return a Tuple. I left the return type as auto, to avoid a global import of std.typecons.

That said, I was surprised to notice that the code ran fine without any import of typecons whatsoever. Must be some 313/314 issue thing. Yet I thought they were fixed?

In any case, I think this should wrap it up? Unless there's anything else you might want me to address?

[schveiguy]

LGTM. Only nit, and I'm not sure if this is correct, but you put: "An $(D std.typecons.Tuple!(...", shouldn't that be "A $(D std.typecons.Tuple!(..." ?

[kyllingstad]

Only nit, and I'm not sure if this is correct, but you put: "An $(D std.typecons.Tuple!(...", shouldn't that be "A $(D std.typecons.Tuple!(..." ?

I've often wondered about this myself. I guess it depends on how you pronounce it? That is, "a standard typecons tuple" versus "an ess tee dee typecons tuple".

[kyllingstad]

I think the whole "Returns" section could be removed, as it no longer provides any additional information beyond what is already in the function signature. Also, further up, in the second paragraph of this doc comment, you could replace

it returns a struct where

with

it returns a tuple where"

[monarchdodra]

I missed the above "struct"/"tuple" thing. Fixed now.

[kyllingstad]

Auto-merge toggled on