Clojure

Audit IReduce usages for proper Reduced handling

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code

Description

Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

Approach: I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

Example: Before the patch:

user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>

Patch: clj-1537-v3.diff
Screened by: Alex Miller

  1. audit-ireduce.diff
    26/Sep/14 11:16 AM
    2 kB
    Timothy Baldridge
  2. clj-1537-gvec-ArraySeq.patch
    14/Nov/14 1:00 PM
    7 kB
    Nicola Mometto
  3. clj-1537-v2.diff
    30/Sep/14 5:59 PM
    2 kB
    Alex Miller
  4. clj-1537-v3.diff
    03/Oct/14 2:23 PM
    2 kB
    Timothy Baldridge

Activity

Timothy Baldridge made changes -
Field Original Value New Value
Description Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Patch:" audit-ireduce.diff
Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Patch:* audit-ireduce.diff
Alex Miller made changes -
Issue Type Enhancement [ 4 ] Defect [ 1 ]
Fix Version/s Release 1.7 [ 10250 ]
Priority Minor [ 4 ] Major [ 3 ]
Approval Vetted [ 10003 ]
Hide
Alex Miller added a comment -

Should be same as audit-ireduce.diff but w/o whitespace diffs.

Show
Alex Miller added a comment - Should be same as audit-ireduce.diff but w/o whitespace diffs.
Alex Miller made changes -
Attachment clj-1537-v2.diff [ 13374 ]
Alex Miller made changes -
Attachment clj-1537-v2.diff [ 13374 ]
Alex Miller made changes -
Attachment clj-1537-v2.diff [ 13375 ]
Alex Miller made changes -
Description Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Patch:* audit-ireduce.diff
Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Patch:* clj-1537-v2.diff
Hide
Alex Miller added a comment -

Should these changes be deref'ing ret?

Also, can you add an example to the description (not sure if it needs to be a test) of where these are an issue?

Show
Alex Miller added a comment - Should these changes be deref'ing ret? Also, can you add an example to the description (not sure if it needs to be a test) of where these are an issue?
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Timothy Baldridge added a comment -

Following the pattern here: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L85 they should deref the reduced value.

Show
Timothy Baldridge added a comment - Following the pattern here: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L85 they should deref the reduced value.
Hide
Timothy Baldridge added a comment -

Failure examples from master, are added to the description.

Show
Timothy Baldridge added a comment - Failure examples from master, are added to the description.
Timothy Baldridge made changes -
Description Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Patch:* clj-1537-v2.diff
Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Example:* Before the patch:

{noformat}
user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>
{noformat}

*Patch:* clj-1537-v2.diff
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Hide
Timothy Baldridge added a comment -

clj-1537-v2.diff didn't properly deref the reduced box. clj-1537-v3.diff does this now.

Show
Timothy Baldridge added a comment - clj-1537-v2.diff didn't properly deref the reduced box. clj-1537-v3.diff does this now.
Timothy Baldridge made changes -
Attachment clj-1537-v3.diff [ 13387 ]
Description Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Example:* Before the patch:

{noformat}
user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>
{noformat}

*Patch:* clj-1537-v2.diff
Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Example:* Before the patch:

{noformat}
user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>
{noformat}

*Patch:* clj-1537-v3.diff
Alex Miller made changes -
Description Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Example:* Before the patch:

{noformat}
user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>
{noformat}

*Patch:* clj-1537-v3.diff
Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

*Approach:* I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

*Example:* Before the patch:

{noformat}
user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>
{noformat}

*Patch:* clj-1537-v3.diff
*Screened by:* Alex Miller
Approval Vetted [ 10003 ] Screened [ 10004 ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Status Open [ 1 ] Closed [ 6 ]
Resolution Completed [ 1 ]
Hide
Nicola Mometto added a comment -

I've reopened this issue as there are still instances of IReduce implementations that don't handle Reduced:

user=> (transduce (take 1) conj (seq (long-array [1 2 3 4])))
#<Reduced@38f774f8: [1]>
Show
Nicola Mometto added a comment - I've reopened this issue as there are still instances of IReduce implementations that don't handle Reduced:
user=> (transduce (take 1) conj (seq (long-array [1 2 3 4])))
#<Reduced@38f774f8: [1]>
Nicola Mometto made changes -
Resolution Completed [ 1 ]
Status Closed [ 6 ] Reopened [ 4 ]
Approval Ok [ 10007 ]
Hide
Nicola Mometto added a comment -

The attached patch (clj-1537-gvec-ArraySeq.patch) fixes the remaining IReduce impls that don't correctly handle Reduced

Show
Nicola Mometto added a comment - The attached patch (clj-1537-gvec-ArraySeq.patch) fixes the remaining IReduce impls that don't correctly handle Reduced
Nicola Mometto made changes -
Attachment clj-1537-gvec-ArraySeq.patch [ 13518 ]
Hide
Nicola Mometto added a comment -

I'm closing this ticket again and opening a different ticket for the new patch, as asked privately by Alex Miller

Show
Nicola Mometto added a comment - I'm closing this ticket again and opening a different ticket for the new patch, as asked privately by Alex Miller
Nicola Mometto made changes -
Status Reopened [ 4 ] Closed [ 6 ]
Resolution Completed [ 1 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: