Clojure

unsigned-bit-shift-right

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Add a clojure equivalent of >>>.

The patch implements a new unsigned-bit-shift-right function that has the semantics of Java long >>>. This differs in particular in the handling of the shift distance which uses the least significant 6 bits of the shift distance (thus always in the range 0-63 inclusive). A Java method Numbers.unsignedShiftRightInt is included but not used by the surfaced function.

Patch: clj-827-unsigned-bit-shift-right-with-tests.patch

Screened by: Alex Miller

Related ticket: Ticket CLJS-514 has been created to track the future work of making ClojureScript's name for this operation the same as the one used in Clojure, after this ticket is completed. David Nolen is agreeable to making this change, even though it would rename an already-defined public function.

Activity

Hide
Joe Gallo added a comment -

I just realized (with the asssistance of Paul Stadig) that just doing only longs is probably sufficient, as you can get the integer version if you really want it:

> (int (bit-and Integer/MAX_VALUE (unsigned-bit-shift-right -5 1)))
2147483645

Of course, that's less efficient than just doing it directly with java, but it's enough that I think my concern from the previous comment is addressed.

Show
Joe Gallo added a comment - I just realized (with the asssistance of Paul Stadig) that just doing only longs is probably sufficient, as you can get the integer version if you really want it: > (int (bit-and Integer/MAX_VALUE (unsigned-bit-shift-right -5 1))) 2147483645 Of course, that's less efficient than just doing it directly with java, but it's enough that I think my concern from the previous comment is addressed.
Joe Gallo made changes -
Field Original Value New Value
Attachment 0001-add-unsigned-bit-shift-right.patch [ 10694 ]
Hide
Tim McCormack added a comment - - edited

I have attached "0001-CLJ-827-Add-bit-shift-right-logical.patch", which implements a logical bit-shift-right using the same JVM bytecode as >>>.

The patch mimics the implementations of << and >>.

Show
Tim McCormack added a comment - - edited I have attached "0001-CLJ-827-Add-bit-shift-right-logical.patch", which implements a logical bit-shift-right using the same JVM bytecode as >>>. The patch mimics the implementations of << and >>.
Tim McCormack made changes -
Rich Hickey made changes -
Approval Vetted [ 10003 ]
Hide
Stuart Halloway added a comment -

For context, this feature appears to be needed for Clojure-in-Clojure data structures: https://groups.google.com/d/msg/clojure-dev/iAwH7CLSFzE/6wzDH4RS1YQJ

Show
Stuart Halloway added a comment - For context, this feature appears to be needed for Clojure-in-Clojure data structures: https://groups.google.com/d/msg/clojure-dev/iAwH7CLSFzE/6wzDH4RS1YQJ
Hide
Michał Marczyk added a comment -

Just wanted to note that I've introduced this operation to ClojureScript when implementing PersistentHashMap. The name over there is bit-shift-right-zero-fill. Would it be alright for Clojure to use that name? Failing that, ClojureScript would probably have to change to match.

Show
Michał Marczyk added a comment - Just wanted to note that I've introduced this operation to ClojureScript when implementing PersistentHashMap. The name over there is bit-shift-right-zero-fill. Would it be alright for Clojure to use that name? Failing that, ClojureScript would probably have to change to match.
Rich Hickey made changes -
Fix Version/s Release 1.6 [ 10157 ]
Gabriel Horner made changes -
Assignee Gabriel Horner [ cldwalker ]
Gabriel Horner made changes -
Hide
Gabriel Horner added a comment -

I've added clj-827-unsigned-bit-shift-right-with-tests.patch which builds off of Tim's patch and adds tests. I also renamed the new fn to unsigned-bit-shift-right after chatting with Rich.

@Michal - This may mean you need to rename the cljs fn. After this lands on master, I can open a ticket with a patch if you'd like.

Show
Gabriel Horner added a comment - I've added clj-827-unsigned-bit-shift-right-with-tests.patch which builds off of Tim's patch and adds tests. I also renamed the new fn to unsigned-bit-shift-right after chatting with Rich. @Michal - This may mean you need to rename the cljs fn. After this lands on master, I can open a ticket with a patch if you'd like.
Gabriel Horner made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Assignee Gabriel Horner [ cldwalker ]
Hide
Michał Marczyk added a comment -

@Gabriel: Thanks! bit-shift-right-zero-fill is not marked private in ClojureScript, so it'll be a breaking change; if it's going to happen eventually (that is, if the name is settled), I suppose it's best to make it without waiting for Clojure.

At any rate, I went ahead and created CLJS-514 to track the coordination effort. No patch attached; if you'd like to do the patching on both sides, that would be cool, otherwise I'll be happy to provide the CLJS patch.

Show
Michał Marczyk added a comment - @Gabriel: Thanks! bit-shift-right-zero-fill is not marked private in ClojureScript, so it'll be a breaking change; if it's going to happen eventually (that is, if the name is settled), I suppose it's best to make it without waiting for Clojure. At any rate, I went ahead and created CLJS-514 to track the coordination effort. No patch attached; if you'd like to do the patching on both sides, that would be cool, otherwise I'll be happy to provide the CLJS patch.
Hide
Gabriel Horner added a comment -

@Michal - I didn't mark it private. Are we taking about the same with-tests.patch? I'll try to get around to the cljs patch but feel free to beat me to it if I'm slow.

Show
Gabriel Horner added a comment - @Michal - I didn't mark it private. Are we taking about the same with-tests.patch? I'll try to get around to the cljs patch but feel free to beat me to it if I'm slow.
Hide
Michał Marczyk added a comment -

@Gabriel: I mean it's not marked private in ClojureScript, so ClojureScript already exports the ...-zero-fill name, technically speaking. As per David Nolen's comment on CLJS-514, this is probably not a big deal.

Show
Michał Marczyk added a comment - @Gabriel: I mean it's not marked private in ClojureScript, so ClojureScript already exports the ...-zero-fill name, technically speaking. As per David Nolen's comment on CLJS-514, this is probably not a big deal.
Hide
Alex Miller added a comment -

It's unclear to me what patches are in play and whether the Clojurescript question is resolved in this ticket. Can someone clean it up? Moving to Incomplete.

Show
Alex Miller added a comment - It's unclear to me what patches are in play and whether the Clojurescript question is resolved in this ticket. Can someone clean it up? Moving to Incomplete.
Alex Miller made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Hide
Michał Marczyk added a comment -

AFAICT the newest two patches are relevant (Gabriel's patch – the latest one – applies on top of Tim's patch – second latest); the first one must have been left around as per the usual Clojure JIRA custom to leave historical patches in place.

As for ClojureScript, Gabriel's comment indicates that Rich expressed a preference for the name unsigned-bit-shift-right. ClojureScript can adjust to that no problem, in fact I think we should just switch the name ahead of time. There is no open question on the ClojureScript side except if Clojure actually merged the patches we'd be able to make any final adjustments and close the CLJS ticket.

NB. the adjustments to make there are only about the name of the function anyway, as explained in the comments above.

I'll also note that the present couple of patches (1) touch no existing behaviour, (2) add in a missing basic and useful operation which we cannot just simulate in Clojure if performance is a concern at all. Hence my long-standing +1 on this issue.

Show
Michał Marczyk added a comment - AFAICT the newest two patches are relevant (Gabriel's patch – the latest one – applies on top of Tim's patch – second latest); the first one must have been left around as per the usual Clojure JIRA custom to leave historical patches in place. As for ClojureScript, Gabriel's comment indicates that Rich expressed a preference for the name unsigned-bit-shift-right. ClojureScript can adjust to that no problem, in fact I think we should just switch the name ahead of time. There is no open question on the ClojureScript side except if Clojure actually merged the patches we'd be able to make any final adjustments and close the CLJS ticket. NB. the adjustments to make there are only about the name of the function anyway, as explained in the comments above. I'll also note that the present couple of patches (1) touch no existing behaviour, (2) add in a missing basic and useful operation which we cannot just simulate in Clojure if performance is a concern at all. Hence my long-standing +1 on this issue.
Andy Fingerhut made changes -
Patch Code [ 10001 ] Code and Test [ 10002 ]
Description Add a clojure equivalent of >>>.

A simple version of this is implemented here (https://github.com/joegallo/clojure/tree/unsigned-bit-shift-right), and just follows the example set by shift-right.

The downside of this implementation is that it treats all integer types as longs, and shifts them accordingly, which yields different results than you would get in java. A previous version of this did not have the same problem, when BitOps was its own thing. I'm not sure if this limitation is acceptable and appropriate, or needs to be worked around (my inclination is the latter).
Add a clojure equivalent of >>>.

A simple version of this is implemented here (https://github.com/joegallo/clojure/tree/unsigned-bit-shift-right), and just follows the example set by shift-right.

The downside of this implementation is that it treats all integer types as longs, and shifts them accordingly, which yields different results than you would get in java. A previous version of this did not have the same problem, when BitOps was its own thing. I'm not sure if this limitation is acceptable and appropriate, or needs to be worked around (my inclination is the latter).

Patch: clj-827-unsigned-bit-shift-right-with-tests.patch

The patch consists of 3 commits, the first of which is identical to the commit in patch 0001-CLJ-827-Add-bit-shift-right-logical.patch dated Jan 16 2012, preserving Tim McCormack's authorship. The 2nd commit changes the name of the new operation to unsigned-bit-shift-right, which Gabriel Horner says in a comment on May 24 2013 that he chose after chatting with Rich. The 3rd commit adds tests for the new unsigned-bit-shift-right, plus one for bit-shift-right that distinguishes the behavior of the two operations.

Ticket CLJS-514 has been created to track the future work of making ClojureScript's name for this operation the same as the one used in Clojure, after this ticket is completed. David Nolen is agreeable to making this change, even though it would rename an already-defined public function.
Hide
Andy Fingerhut added a comment -

I made some additions to the description on the one patch that I think should be considered, and why, and comments on what appears to be the current state of the ClojureScript question. Feel free to edit further if it needs it.

Show
Andy Fingerhut added a comment - I made some additions to the description on the one patch that I think should be considered, and why, and comments on what appears to be the current state of the ClojureScript question. Feel free to edit further if it needs it.
Alex Miller made changes -
Labels math
Hide
Alex Miller added a comment -

For reference, notes on how Java does >>> for ints and longs is here:
http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19

I found that section particularly helpful wrt understanding the behavior of the shift distance parameter. Namely, that for longs the shift distance uses only the least significant 6 bits (bit-and with 0x3f) which explains some of the added tests using large or negative shift distances.

Show
Alex Miller added a comment - For reference, notes on how Java does >>> for ints and longs is here: http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19 I found that section particularly helpful wrt understanding the behavior of the shift distance parameter. Namely, that for longs the shift distance uses only the least significant 6 bits (bit-and with 0x3f) which explains some of the added tests using large or negative shift distances.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Description Add a clojure equivalent of >>>.

A simple version of this is implemented here (https://github.com/joegallo/clojure/tree/unsigned-bit-shift-right), and just follows the example set by shift-right.

The downside of this implementation is that it treats all integer types as longs, and shifts them accordingly, which yields different results than you would get in java. A previous version of this did not have the same problem, when BitOps was its own thing. I'm not sure if this limitation is acceptable and appropriate, or needs to be worked around (my inclination is the latter).

Patch: clj-827-unsigned-bit-shift-right-with-tests.patch

The patch consists of 3 commits, the first of which is identical to the commit in patch 0001-CLJ-827-Add-bit-shift-right-logical.patch dated Jan 16 2012, preserving Tim McCormack's authorship. The 2nd commit changes the name of the new operation to unsigned-bit-shift-right, which Gabriel Horner says in a comment on May 24 2013 that he chose after chatting with Rich. The 3rd commit adds tests for the new unsigned-bit-shift-right, plus one for bit-shift-right that distinguishes the behavior of the two operations.

Ticket CLJS-514 has been created to track the future work of making ClojureScript's name for this operation the same as the one used in Clojure, after this ticket is completed. David Nolen is agreeable to making this change, even though it would rename an already-defined public function.
Add a clojure equivalent of >>>.

The patch implements a new {{unsigned-bit-shift-right}} function that has the semantics of Java long >>>. This differs in particular in the handling of the shift distance which uses the least significant 6 bits of the shift distance (thus always in the range 0-63 inclusive). A Java method {{Numbers.unsignedShiftRightInt}} is included but not used by the surfaced function.

*Patch:* clj-827-unsigned-bit-shift-right-with-tests.patch

*Screened by:* Alex Miller

*Related ticket:* Ticket CLJS-514 has been created to track the future work of making ClojureScript's name for this operation the same as the one used in Clojure, after this ticket is completed. David Nolen is agreeable to making this change, even though it would rename an already-defined public function.
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (11)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: