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

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.
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?
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.
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.
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]>
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
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

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: