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.

Sign up for GitHub

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

Jump to bottom

Adhere to Base.copy!(x, i, y, j, n) behaviour for n <= 0 #111

Merged
merged 2 commits into from Dec 14, 2017
Merged

Adhere to Base.copy!(x, i, y, j, n) behaviour for n <= 0 #111

merged 2 commits into from Dec 14, 2017

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Dec 12, 2017

This is consistent with Base copy!(x, i, y, j, 0): no checks for i or j are done.
IRL it's observed during join(df1, df2, kind=:inner) when the result contains no rows.

@alyst
Copy link
Contributor Author

alyst commented Dec 12, 2017

The usual Date CI failures seem unrelated.

@nalimilan
Copy link
Member

Why not, if that's consistent with Base. Can you put the tests in a testset and make comments more explicit about the fact that we test that no error is thrown for out of bounds indices?

@alyst
copy!(): skip index checks if length is 0
a7377ba 
this is consistent with Array copy!()
@alyst
Copy link
Contributor Author

alyst commented Dec 13, 2017

added testsets

@alyst alyst mentioned this pull request Dec 13, 2017
Merged
@alyst
Copy link
Contributor Author

alyst commented Dec 13, 2017

Sorry for pushing another commit after the approval, but it's related, so I though it should go in the same PR.
It actually improves the conformance to Base copy!(): the negative length (n) should be checked before the indices checks, but not in the case of the default n.
Otherwise BoundsError may confuse the user by referencing the copy range that the user never specified.

@alyst alyst changed the title No index checks for 0-length copy!() Adhere to Base.copy!(x, i, y, j, n) behaviour for n <= 0 Dec 13, 2017
nalimilan
nalimilan reviewed Dec 13, 2017
View reviewed changes
src/array.jl Outdated
@@ -396,14 +396,21 @@ copy(A::CategoricalArray) = deepcopy(A)
CatArrOrSub{T, N} = Union{CategoricalArray{T, N},
SubArray{<:Any, N, <:CategoricalArray{T}}} where {T, N}

function copy!(dest::CatArrOrSub, dstart::Integer,
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT we don't need to define this method, we can rely on the AbstractArray fallback. Just remove the default value of n in the method below.

@alyst
copy!(): move n < 0 check before bounds checks
a021609 
because n<0 is likely to trigger BoundsError, but the message would be
confusing (attempt to access n-element Array at index [m:m-1])

use Base's copy!(x, i, y, j) for the default n
@alyst
Copy link
Contributor Author

alyst commented Dec 14, 2017

CI passes on 0.6

@nalimilan nalimilan merged commit df86ae9 into JuliaData:master Dec 14, 2017
@alyst alyst deleted the copy_0len branch January 8, 2018 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alyst @nalimilan