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

Allow bitarray to handle shifts by a negative value #18154

Closed
wants to merge 2 commits into from

Conversation

dbeach24
Copy link
Contributor

Fix for #16706.
This allows left/right shifting a bitarray by a negative value.

@dbeach24
Copy link
Contributor Author

@eschnett Could you please verify that this does what you wanted?

@yuyichao yuyichao changed the title Djcb/bugfix 16706 Allow bitarray to handle shifts by a negative value Aug 25, 2016
@dbeach24
Copy link
Contributor Author

Is this merge-worthy?

@tkelman
Copy link
Contributor

tkelman commented Aug 29, 2016

LGTM, though the merge commit should be squashed out (at merge time if needed)

@test isequal(b1 >>> m, [ falses(m); b1[1:end-m] ])
@test isequal(b1 >>> -m, b1 << m)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, for completeness, add a test for b1 >> -m ?

Copy link
Contributor Author

@dbeach24 dbeach24 Aug 30, 2016

Choose a reason for hiding this comment

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

Done

@rfourquet
Copy link
Member

LGTM too.

@rfourquet
Copy link
Member

Also while you are at it, you could update the docs for shift operations (e.g. the doc for >> implicitly assumes that the LHS is an integer type (with a sign)). Otherwise let's open a doc issue for that.

@dbeach24
Copy link
Contributor Author

@rfourquet Not clear on how to update the docs. The docstrings for the shift operators are found in base/operators.jl, where the methods are defined for integer types. I don't want to change the signature of these functions (since they are specific to Integer).

Can you clarify what I'm supposed to do here?

@rfourquet
Copy link
Member

I think it would be enough to attach additional docstrings to the shift methods acting on bitarrays.

@kshyatt
Copy link
Contributor

kshyatt commented Sep 14, 2016

I'm bumping this - can we merge this as-is and deal with the docs later? Alternatively, can someone guide @dbeach24 through the process of adding the docstrings?

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Sep 14, 2016
@rfourquet
Copy link
Member

I think it can, but would be nice to at least open a doc issue.

@tkelman
Copy link
Contributor

tkelman commented Dec 29, 2016

replaced by #19760

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

Successfully merging this pull request may close these issues.

6 participants