Make falses(A) and trues(A) not change their argument #15827
Conversation
| @@ -236,7 +236,9 @@ function fill!(B::BitArray, x) | |||
| end | |||
|
|
|||
| falses(args...) = fill!(BitArray(args...), false) | |||
There was a problem hiding this comment.
I think we should also restrict the signature here. As this PR shows, too broad signatures can give unexpected results. Looks like we need two methods: falses(::Dims) and falses(::Integer...).
There was a problem hiding this comment.
Good idea. Updated accordingly and DRYed up.
martinholters
force-pushed
the
fix_trues_falses
branch
from
99d9dea
to
62624b7
Compare
| @@ -235,8 +235,11 @@ function fill!(B::BitArray, x) | |||
| return B | |||
| end | |||
|
|
|||
| falses(args...) = fill!(BitArray(args...), false) | |||
| trues(args...) = fill!(BitArray(args...), true) | |||
| for (f, v) in ((:falses, false), (:trues, true)) | |||
There was a problem hiding this comment.
Honestly, I don't think the @eval machinery is worth it here. It makes the code much harder to read.
Also, do you know if both variants are currently tested?
There was a problem hiding this comment.
Honestly, I don't think the @eval machinery is worth it here. It makes the code much harder to read.
After making two of the cases just call the third instead of all of them calling fill!, I couldn't stop myself. But you're probably right.
Also, do you know if both variants are currently tested?
No, but even if both of them get implicitly tested somewhere by just using them, it's probably a good idea to explicitly test them as part of the tests of this PR.
martinholters
force-pushed
the
fix_trues_falses
branch
from
62624b7
to
4ad278c
Compare
|
|
||
| falses(A) | ||
|
|
||
| Create a `BitArray` with all values set to `false` of the same shape as `A`. | ||
| """ | ||
| falses |
There was a problem hiding this comment.
move the docstrings inline while you're at it?
There was a problem hiding this comment.
Good opportunity indeed, will do so tomorrow.
|
Thanks! This is indeed a bad bug. |
martinholters
force-pushed
the
fix_trues_falses
branch
2 times, most recently
from
c84a48c
to
e4a1bbd
Compare
martinholters
changed the title
| falses(A) | ||
|
|
||
| Create a `BitArray` with all values set to `false` of the same shape as `A`. | ||
| """ |
There was a problem hiding this comment.
may as well separate the docstrings for the separate signatures, I think
There was a problem hiding this comment.
Um, yes, just tried to, but then genstdlib.jl does not include the docs for the second signature in its output. Any idea what I'm doing wrong? The REPL output with "?" looks the same as before.
There was a problem hiding this comment.
You need to manually add the first line with the signature into the rst for genstdlib to match it and populate the rest
martinholters
force-pushed
the
fix_trues_falses
branch
from
e4a1bbd
to
ef93270
Compare
Make falses(A::BitArray) and trues(A::BitArray) not change their arguments by implementing falses(A::AbstractArray) and trues(A::AbstractArray) and replacing the completely generic case with more specific ones for ::Dims and ::Interger... arguments. Update documentation accordingly and move it inline while at it.
martinholters
force-pushed
the
fix_trues_falses
branch
from
ef93270
to
9523972
Compare
|
Could be merged, no? |
Make falses(A::BitArray) and trues(A::BitArray) not change their arguments by implementing falses(A::AbstractArray) and trues(A::AbstractArray) and replacing the completely generic case with more specific ones for ::Dims and ::Interger... arguments. Update documentation accordingly and move it inline while at it. (cherry picked from commit 9523972) ref #15827
Presently, we have:
which is undocumented and certainly surprising. This PR makes
falses(A)andtrues(A)not change their argument and documents this syntax.