Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add std.range.choose() #3269

Merged
merged 1 commit into from May 22, 2015
Merged

add std.range.choose() #3269

merged 1 commit into from May 22, 2015

Conversation

WalterBright
Copy link
Member

Phobos has all sorts of algorithms for looping, but none for selection. choose plugs that gap, and can be used for ranges where one would use if-else, ?:, and switch for expressions.

Despite the number of lines in this, it is mostly just copy/pasta code from its cousin, chain.

The indexed range. If rs consists of only one range,
the return type is an alias of that range's type.
*/
auto choose(Ranges...)(size_t index, Ranges rs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should specialize for Ranges.length == 2 and type of index == bool. That would improve correctness for a common case. BTW in other places (e.g. among or startsWith) we use uint, not size_t, for variadic counts so you may want to continue the tradition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but if(c) e1 else e2 has the e1 and e2 reversed from the way they are if there are n arguments. I worry that the specialization would be very confusing.

Also, I used size_t for the index to emphasize its commonality with indexing an array. Calculations of such an index will likely be done with size_t's, and then narrowing them to a uint would require the user to insert an ugly cast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's precedent: Select and select. http://dlang.org/phobos/std_traits.html#Select. But point taken.

@andralex
Copy link
Member

andralex commented May 9, 2015

ok let's fix the doc and pull this

}
}

static if (allSatisfy!(hasLength, R) && allSatisfy!(hasSlicing, R))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lets R = TypeTuple!(int[], long[]) through, but shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R is already checked for being a range.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void main()
{
    import std.range: choose;
    int[] a;
    long[] b;
    auto c = choose(0, a, b);
}

That should compile and c should not have an opSlice. At the moment, it fails compilation with std/range/package.d(1532): Error: mismatched function return type inference of long[] and int[].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Fixed.

@andralex
Copy link
Member

@WalterBright per our private discussion, shall we go with choose as an equivalent for if and chooseAmong as an equivalent for switch? Experience suggests choose with two branches covers most cases of interest and is intuitive by similarity with if.

@JakobOvrum
Copy link
Member

Why should the two-range version have a different name? It would make choose[Among] clumsy to use correctly in generic code.

@WalterBright
Copy link
Member Author

The issue is if we think of choose() as an equivalent for if (b) e0 else e1 and wanted to use choose we'd write it as choose(b != 0, e1, e0). Note the reversal of the operands. This reversal, along with the bool test not being the same as an index, means that this usage cannot be reasonably overloaded with the indexing version.
Most usages of choose would likely be the if``replacement, not theswitch` replacement, even though the latter is more general.
The question becomes, then, does this justify having a separate algorithm for the 2 range case?

@andralex
Copy link
Member

Auto-merge toggled on

@andralex
Copy link
Member

pulling this now so I can work on it

andralex added a commit that referenced this pull request May 22, 2015
@andralex andralex merged commit a1e3944 into dlang:master May 22, 2015
@WalterBright WalterBright deleted the choose branch May 22, 2015 06:17
@MartinNowak
Copy link
Member

What about using a range as element-wise selector?
The documentation doesn't explain why this function is needed.
A meaningful example would help a lot.

@MartinNowak
Copy link
Member

Seriously, this is implemented non-lazily?
How am I supposed to create the non-needed values?
Has anyone even tried to use this function beyond the immediate need?

@quickfur
Copy link
Member

What do you mean by "implemented non-lazily"? The returned range is certainly lazy, AFAICT.

Or are you referring to the fact that the choice of range is determined at call time, as opposed to switching between the given ranges while the resulting range is being iterated over? For the latter case, I have found need for it before; if you'll file an enhancement request, I'll see if I can whip up an implementation.

@quickfur
Copy link
Member

In the meantime, the docs should be fixed: #3653

@MartinNowak
Copy link
Member

I mean this doesn't behave like a ternary operator where only one leave is evaluated, instead I have to construct and pass 2 ranges, but in many use-cases one of those ranges (one branch of the ternary operator) isn't valid.

// constructor of TrueRNG fails b/c hasTrueRNG is false
choose(hasTrueRNG, TrueRNG(), PseudoRNG())

@quickfur
Copy link
Member

Oh. So you're saying the range arguments should be lazy? If so, why bother using choose over, say, static if or just a ternary operator?

I think this function was intended to solve the case where, based on some given condition, you need to choose between ranges of different types, that you can't just return from a function or assign to a common variable, because they are incompatible types (e.g., one is the result of filter and the other is the result of map). I don't think it was intended to replace the ternary operator. The argument ranges may have already been constructed at that point.

@MartinNowak
Copy link
Member

The types are different and the decision can only be made at runtime, so it's exactly what choose was made for. The example was a bit contrieved here is what my code looked like.

choose(data["kind"] == "set", data["tracks"].get!(Json[]).map!(t => t.get!(Json[string]).filter!pred, only(data).filter!pred);

Creating a range is done eagerly and might already do work which isn't possible based on the condition, in this example there is no data["tracks"].

@JakobOvrum
Copy link
Member

Oh. So you're saying the range arguments should be lazy? If so, why bother using choose over, say, static if or just a ternary operator?

The ternary operator is not an option because it requires the two resulting operands to have a common type, and static-if is not an option because the whole point is to make the decision at runtime.

I agree the range parameters should be lazy. Did we fix attribute inference with lazy parameters yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants