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

RFC: Make any and all always short-circuit #19543

Merged
merged 5 commits into from
Dec 17, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ Library improvements

* New `titlecase` function, which capitalizes the first character of each word within a string ([#19469]).

* `any` and `all` now always short-circuit, and `mapreduce` never short-circuits ([#19543]).
Copy link
Member

Choose a reason for hiding this comment

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

Could also mention reduce here, since it is equally affected by the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, added. Thanks!

That is, not every member of the input iterable will be visited if a `true` (in the case of `any`) or
`false` (in the case of `all`) value is found, and `mapreduce` will visit all members of the iterable.

Compiler/Runtime improvements
-----------------------------

Expand Down
2 changes: 1 addition & 1 deletion base/char.jl
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ length(c::Char) = 1
endof(c::Char) = 1
getindex(c::Char) = c
getindex(c::Char, i::Integer) = i == 1 ? c : throw(BoundsError())
getindex(c::Char, I::Integer...) = all(Predicate(x -> x == 1), I) ? c : throw(BoundsError())
getindex(c::Char, I::Integer...) = all(x -> x == 1, I) ? c : throw(BoundsError())
first(c::Char) = c
last(c::Char) = c
eltype(::Type{Char}) = Char
Expand Down
110 changes: 39 additions & 71 deletions base/reduce.jl
Original file line number Diff line number Diff line change
Expand Up @@ -283,54 +283,6 @@ determine the neutral element of `op`.
reduce(op, itr) = mapreduce(identity, op, itr)
reduce(op, a::Number) = a

### short-circuiting specializations of mapreduce

## conditions and results of short-circuiting

immutable Predicate{F}
f::F
end
(pred::Predicate)(x) = pred.f(x)::Bool

const ShortCircuiting = Union{typeof(&), typeof(|)}

## short-circuiting (sc) mapreduce definitions

function mapreduce_sc_impl(f, op::typeof(&), itr)
for x in itr
f(x) || return false
end
return true
end

function mapreduce_sc_impl(f, op::typeof(|), itr)
for x in itr
f(x) && return true
end
return false
end

# mapreduce_sc tests if short-circuiting is safe;
# if so, mapreduce_sc_impl is called. If it's not
# safe, call mapreduce_no_sc, which redirects to
# non-short-circuiting definitions.

mapreduce_no_sc(f, op, itr::Any) = mapfoldl(f, op, itr)
mapreduce_no_sc(f, op, itr::AbstractArray) = _mapreduce(f, op, itr)

mapreduce_sc(f::Function, op, itr) = mapreduce_no_sc(f, op, itr)
mapreduce_sc(f::Predicate, op, itr) = mapreduce_sc_impl(f, op, itr)

mapreduce_sc(f::typeof(identity), op, itr) =
eltype(itr) <: Bool ?
mapreduce_sc_impl(f, op, itr) :
mapreduce_no_sc(f, op, itr)

mapreduce(f, op::ShortCircuiting, n::Number) = n
mapreduce(f, op::ShortCircuiting, itr::AbstractArray) = mapreduce_sc(f,op,itr)
mapreduce(f, op::ShortCircuiting, itr::Any) = mapreduce_sc(f,op,itr)


###### Specific reduction functions ######

## sum
Expand Down Expand Up @@ -524,6 +476,7 @@ end
any(itr) -> Bool

Test whether any elements of a boolean collection are `true`.
Not all items in `itr` will be visited if a `true` value is found.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "Returns true as soon as the first true value is found in itr (short circuiting)." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mix of imperative ("test whether") and non-imperative ("returns true") seems a little odd to me but maybe it's fine. How about something like

Test whether any elements of a boolean collection are true, returning true as soon as the first true value in itr is encountered (short-circuiting).

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, went with that then. Thanks!


```jldoctest
julia> a = [true,false,false,true]
Expand All @@ -535,6 +488,10 @@ julia> a = [true,false,false,true]

julia> any(a)
true

julia> any((println(i); v) for (i, v) in enumerate(a))
1
true
```
"""
any(itr) = any(identity, itr)
Expand All @@ -543,6 +500,7 @@ any(itr) = any(identity, itr)
all(itr) -> Bool

Test whether all elements of a boolean collection are `true`.
Not all items in `itr` will be visited if a `false` value is found.

```jldoctest
julia> a = [true,false,false,true]
Expand All @@ -554,53 +512,63 @@ julia> a = [true,false,false,true]

julia> all(a)
false

julia> all((println(i); v) for (i, v) in enumerate(a))
1
2
false
```
"""
all(itr) = all(identity, itr)

nonboolean_error(f, op) = throw(ArgumentError("""
Using non-boolean collections with $f(itr) is not allowed, use
reduce($op, itr) instead. If you are using $f(map(f, itr)) or
$f([f(x) for x in itr]), use $f(f, itr) instead.
"""))
or_bool_only(a, b) = nonboolean_error(:any, :|)
or_bool_only(a::Bool, b::Bool) = a|b
and_bool_only(a, b) = nonboolean_error(:all, :&)
and_bool_only(a::Bool, b::Bool) = a&b

"""
any(p, itr) -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention short-circuiting behaviour in the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

with an example, e.g.:

any(i -> (println(i); i > 3), 1:10)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Look okay? Thanks!


Determine whether predicate `p` returns `true` for any elements of `itr`.
Not all items in `itr` will be visited if a `true` value is found.

```jldoctest
julia> any(i->(4<=i<=6), [3,5,7])
true

julia> any(i -> (println(i); i > 3), 1:10)
1
2
3
4
true
```
"""
any(f::Any, itr) = any(Predicate(f), itr)
any(f::Predicate, itr) = mapreduce_sc_impl(f, |, itr)
any(f::typeof(identity), itr) =
eltype(itr) <: Bool ?
mapreduce_sc_impl(f, |, itr) :
reduce(or_bool_only, false, itr)
function any(f, itr)
for x in itr
f(x) && return true
end
return false
end

"""
all(p, itr) -> Bool

Determine whether predicate `p` returns `true` for all elements of `itr`.
Not all items in `itr` will be visited if a `false` value is found.

```jldoctest
julia> all(i->(4<=i<=6), [4,5,6])
true

julia> all(i -> (println(i); i < 3), 1:10)
1
2
3
false
```
"""
all(f::Any, itr) = all(Predicate(f), itr)
all(f::Predicate, itr) = mapreduce_sc_impl(f, &, itr)
all(f::typeof(identity), itr) =
eltype(itr) <: Bool ?
mapreduce_sc_impl(f, &, itr) :
reduce(and_bool_only, true, itr)
function all(f, itr)
for x in itr
f(x) || return false
end
return true
end

## in & contains

Expand Down Expand Up @@ -629,7 +597,7 @@ julia> 5 in a
false
```
"""
in(x, itr) = any(Predicate(y -> y == x), itr)
in(x, itr) = any(y -> y == x, itr)

const ∈ = in
∉(x, itr)=!∈(x, itr)
Expand Down
9 changes: 9 additions & 0 deletions test/reduce.jl
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,15 @@ let c = [0, 0], A = 1:1000
@test c == [10,10]
end

# 19151 - always short circuit
let c = Int[], d = Int[], A = 1:9
all((push!(c, x); x < 5) for x in A)
@test c == collect(1:5)

any((push!(d, x); x > 4) for x in A)
@test d == collect(1:5)
end

# any and all with functors

immutable SomeFunctor end
Expand Down