<< Back to previous view

[CLJ-1571] Transducer of partition-by over take gives wrong answer Created: 20/Oct/14  Updated: 21/Oct/14  Resolved: 21/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 1
Labels: transducers

Attachments: Text File 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch     Text File CLJ-1571.patch    
Approval: Ok

 Description   
(partition-by pos? (take 2 [-1 1]))
=> ((-1) (1))
(sequence (comp (take 2) (partition-by pos?)) [-1 1])
=> ([-1])


 Comments   
Comment by Nicola Mometto [ 21/Oct/14 7:49 AM ]

Given that it works fine when using transduce instead of sequence, the bug might be in LazyTransformer rather than in partition-by.

(into [] (comp (take 2) (partition-by pos?)) [-1 1])
=> [[-1] [1]]
Comment by Ghadi Shayban [ 21/Oct/14 9:21 AM ]

Patch fixes the test case, but needs eyes, I certainly may have broken something. This highlights the importance of CLJ-1554, something similar to the existing defequiv tests for reducers, but between #'into and #'sequence, also covering edge cases in reduced unwrapping.

Comment by Alex Miller [ 21/Oct/14 9:41 AM ]

Thanks Ghadi. This bug was found by the tests I wrote for CLJ-1554, so yes.

Comment by Nicola Mometto [ 21/Oct/14 9:53 AM ]

Applying this patch causes a regression in the lazyiness of sequence.
The lines that Ghadi removed for this patch were added by Rich in this commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c to address http://dev.clojure.org/jira/browse/CLJ-1497

Example of the regression:
current master:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
(1 2)

with this patch:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
~3
(1 2)
Comment by Nicola Mometto [ 21/Oct/14 10:03 AM ]

Patch 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch addresses this issue while preserving the current lazyness factor of `sequence`

Comment by Alex Miller [ 21/Oct/14 11:09 AM ]

Rich has a (different) patch for this on the way.

Comment by Alex Miller [ 21/Oct/14 1:16 PM ]

Fixed directly by Rich in commit https://github.com/clojure/clojure/commit/38d7572e4254afdd7f02b78095ccdb27065754d2





[CLJ-1569] transduce does not respect the init arity of transducers Created: 19/Oct/14  Updated: 20/Oct/14  Resolved: 20/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel James Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: transducers


 Description   

Note: I initially raised this issue for discussion on the mailing list
https://groups.google.com/d/msg/clojure/uVKP4_0KMwQ/-oUJahvUarIJ

transduce and other transducible processes currently ignore the 'init' arity of transducers. The currently implementation of transduce takes the 'init' from the reducing function before being transformed by the transducer, rather the reducing function after being transformed.

The current implementation of transduce is equivalent to the following (simplified for exposition purposes):

Current implementation of transduce
(defn transduce
  ([xform f coll]
     (transduce xform f (f) coll))
  ([xform f init coll]
     (let [rf (xform f)]
       (rf (reduce rf init coll)))))

The arity 3 case uses (f) to construct the seed value of the reduction. The arity 4 case uses the explicitly provided seed, init.

I would like to propose an alternate implementation of transduce, one which makes use of the transducer when seeding the reduction.

Proposed implementation of transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ([xform f init coll]
     (let [rf (xform
               (fn
                 ([] init)
                 ([result] (f result))
                 ([result input] (f result input))))]
       (rf (reduce rf (rf) coll)))))

Now, the arity 3 case uses (xform f) to construct the seed value of the reduction. The arity 4 case combines both f and init into a new reducing function that is given to xform. Both of these ensure that the init arity of the transducer is used.

As into is implemented in terms of transduce, it is also taken care of. However, sequence is separate, and would also have to be tweaked to respect the init arity.



 Comments   
Comment by Daniel James [ 19/Oct/14 1:24 PM ]

As a small addition, I just wanted to point out an example of where the current implementation raised curiosity:
https://groups.google.com/d/msg/clojure/M-13lRPfguc/IspgdpKDaGsJ

Comment by Alex Miller [ 20/Oct/14 9:12 AM ]

In transduce, the transducer is applied to the elements of the input and should not be entangled with the accumulation at all (either in initializing it or the act of accumulation). f is the final reducing function that deals with accumulation and initialization.

Comment by Daniel James [ 20/Oct/14 10:00 AM ]

Hi Alex,

I feel that you've misunderstood my proposal.

Could you explain how you consider

(defn init-with [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input] (rf result input)))))

to be “entangled with the accumulation at all (either in initializing it or the act of accumulation).”

This seems like a completely legitimate transducer to me. It makes use of the init arity, while remaining oblivious to the accumulation.

Your explanation also seems to be at odds with

http://clojure.org/transducers

The inner function is defined with 3 arities used for different purposes:

  • Init (arity 0) - in most cases, this will just call the init arity on the nested transform xf, which will eventually call out to the transducing process to supply an initial value. It is also a place to establish the initial reducing state for the transducer.
Comment by Alex Miller [ 20/Oct/14 11:57 AM ]

By "entangling" I mean that in your alternate transduce you invoke the xform to obtain the initial value: ((xform f)) instead of (f). Transducers should not know about or be involved in the accumulating process.

The transducers page is in error and I will correct it (I wrote it; the error is mine).

Comment by Daniel James [ 20/Oct/14 3:25 PM ]

Ok, at the risk of belaboring the point (I have enough self-awareness to realized that I am probably about to do exactly that…) I feel that you are still missing something here.

Permit me to try one more time to explain my position.

Consider map

the map transducer
(defn map [f]
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input] (rf result (f input))))))

It defines all three arities, init, step, and completion. It doesn’t have anything to do in init arity, and so the only thing it can do is “call the init arity on the nested transform rf, which will eventually call out to the transducing process.” (taken from your update to http://clojure.org/transducers)

Saying that transducers should not be involved in the accumulating process has the right spirit, but you are missing something. It is involved, but in a strictly constrained way. The transducer’s responsibility is to carefully thread the accumulator value around. Sure, it should not know what the value is, or what type it has, but it is still there. Every arity of map has access to it! In the init arity, map delegates to rf to construct it. In the completion arity, map has the result, but the only valid thing it can do with it is to pass it on to rf. Again, in the step arity, map has the result, and again the only legitimate thing it can do with it is to thread to through to rf.

Now consider the identity transducer:

the identity transducer
(def identity
  (fn [rf]
    ([] (rf))
    ([result] (rf result))
    ([result input] (rf result input))))

This is a transducer in its purest form. All it has to do is correctly thread the accumulation value around. It doesn’t and shouldn’t know any details of what that value is, nonetheless, it still has the responsibility of threading that value correctly.

In each arity the identity transducer does the ‘trivial’ thing. In my post to the mailing list, I illustrated three example of transducers that do something beyond the trivial thing in each of the three arities. (I’ll copy them here for completeness.)

non trivial threading of the accumulator in the init arity
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the completion arity
(defn complete-with
  [x]
  (fn [rf]
    (fn
      ([] (rf))
      ([result]
         (rf (rf result x)))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the step arity
(defn dupl
  []
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input]
         (rf (rf result input)
             input)))))

I would consider all of these to be perfectly valid transducers. However, unless I’ve misunderstood, you appear to be taking issue with init-with. If so, I’m very curious as to why!

a closer look at the init arity of init-with
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ...

Rather than just delegating to (rf), it threads that value immediately into rf with (rf (rf) x). So I don’t agree at all that any of these, init-with, complete-with, or dupl, are “entangled” with the accumulation value or the accumulation process. They are completely oblivious to both its value and its type!

So, returning to transduce,

the first case of an alternate transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ...

A valid transducer is one that threads the accumlation value correctly. Therefore, ((xform f)) is (f) threaded through xform. All the transducers in clojure.core have the trivial ([] (rf)), so ((xform f)) built from these core transducers degenerates into (identity (f)).
However, as transduce, into, and sequence never even invoke the init arity, it begs the question, why even require that transducers have that arity in the first place? Personally, I think that init arity is great as it enables a transducer such as init-with (while remaining stateless), but that requires transducible processes to actually make use of the init arity! Hence why I raised this issue.
It seems troubling to me that complete-with works perfectly fine in the current framework, yet init-with, its dual, does not.

I recognize that the various discussions around ‘typing transducers’ have made various approximations at elucidating the properties of transducers, but I feel strongly that the discussions around rank-2 polymorphism have some bearing on exactly this issue. In fact, it says rather a lot about correctly threading the accumulation value throught transducers without ever “entangling” it in the precise accumulation process of where a transducer is being used.

And on this, it appears that Rich Hickey agrees: “The rank-2 type in particular captures an important property.” (http://conscientiousprogrammer.com/blog/2014/08/07/understanding-cloure-transducers-through-types/#comment-1533318972) Maybe I’ve got him all wrong, but as of right now I’m pretty convinced I don’t. Still, I’m willing to be convinced otherwise

Comment by Alex Miller [ 20/Oct/14 10:03 PM ]

Rich asked me to decline the ticket because the init arity of the xform should not be involved in the reducing function accumulation.

Comment by Daniel James [ 20/Oct/14 10:34 PM ]

Ok, as you can guess I’m a little perplexed by that design choice, but I’ll accept it.

I’d appreciate any further insight you can offer on why this design choice has been taken.
Is the init arity simply a case of compatibility, despite it not being used? Is this a case of attempting to prevent the transducer writer from erroneously corrupting a transducible process? Is init-with actually actually considered to be an invalid transducer, and thus the only way to implement something equivalent would be as a stateful transducer?





[CLJ-1564] Sum/sub decimals operation bug Created: 15/Oct/14  Updated: 15/Oct/14  Resolved: 15/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Luca Gugole Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: math

Patch: Code

 Description   

The result of operation (+ 0.7 0.1) is 0.7999999999999999 and not 0.7

Other operations with the same behaviour:
(+ 0.11 0.1) => 0.21000000000000002
(+ 0.31 0.1) => 0.41000000000000003
(- 0.8 0.1) => 0.7000000000000001
(- 0.41 0.1) => 0.30999999999999994



 Comments   
Comment by Oliver Charles [ 15/Oct/14 6:44 AM ]

Uh, isn't this just normal floating point arithmetic?

Comment by Luca Gugole [ 15/Oct/14 7:32 AM ]

But the result of other operations ((+ 0.1 0.1), (+ 0.2 0.2), (+ 0.2 0.3) ...) has only one decimal place.
It's normal?
I have to perform a math round operation to obtain only one decimal place?

Comment by Jozef Wagner [ 15/Oct/14 7:41 AM ]

This is not a bug. Please read What Every Programmer Should Know About Floating-Point Arithmetic

Comment by Luca Gugole [ 15/Oct/14 7:51 AM ]

Sorry about my lack of knowledge on this subject.
Thank you for the answer.





[CLJ-1559] A function bound in let can only be used in a macro if it is parameterless Created: 09/Oct/14  Updated: 10/Oct/14  Resolved: 09/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Colin Smith Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

MacOS


Attachments: File stack.trace    

 Description   

This works:

(defn make-fn [] (fn [x] (+ 3 x)))

(defmacro m [x]
(let [a-function (make-fn)]
`(fn [z#]
(+ ~x (~a-function z#)))))

(prn ((m 1) 2))

;;;;;;;;;;;;;;;;;;

But this does not (adding a parameter to make-fn foils things):

(defn make-fn [y] (fn [x] (+ y x)))

(defmacro m [x]
(let [a-function (make-fn 3)]
`(fn [z#]
(+ ~x (~a-function z#)))))

(prn ((m 1) 2))

;;;;;;;;;;;;;;;;;;;

stack trace attached.



 Comments   
Comment by Alex Miller [ 09/Oct/14 11:51 PM ]

At a glance, you appear to be dropping the evaluated function object (a-function) inside the syntax quote, which is pretty much always a problem in how the macro is written.

Probably really want something like:

(defn make-fn [y] (fn [x] (+ y x)))
(defmacro m [x] 
  `(let [a-function# (make-fn 3)]
     (fn [z#] (+ ~x (a-function# z#)))))
(prn ((m 1) 2))
Comment by Alex Miller [ 09/Oct/14 11:53 PM ]

If you look at the expanded macro in your example:

user=> (pprint (clojure.walk/macroexpand-all '(prn ((m 1) 2))))
(prn
 ((fn*
   ([z__25__auto__]
    (clojure.core/+
     1
     (#<user$make_fn$fn__22 user$make_fn$fn__22@72995b29>
      z__25__auto__))))
  2))

Any time you see something like #<user$make_fn$fn_22 user$make_fn$fn_22@72995b29>, that's a good hint.

Comment by Colin Smith [ 10/Oct/14 1:21 AM ]

Thank you Alex. I had done the macro expansion.

I see my mistake: I am coming to Clojure from Scheme, where the result of (lambda ...) is a primitive value that I could expect to paste into the form produced by a macro. Now, I get the impression that (fn) is not required to produce quite that sort of thing.
Would you say that's the right way to look at it?

Thanks for looking at this so quickly! I am back on track.





[CLJ-1558] lazy-seq and seq return different values for (lazy-seq []) (seq []) Created: 09/Oct/14  Updated: 09/Oct/14  Resolved: 09/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jeremy Betts Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(lazy-seq [])
=> ()
(seq [])
=> nil

would expect both to return nil



 Comments   
Comment by Andy Fingerhut [ 09/Oct/14 10:02 AM ]

Jeremy, lazy-seq's documentation string says it: "returns a Seqable object that will invoke the body only the first time seq is called". Even (lazy-seq nil) does not return nil. seq's documentation string explicitly says that it will return nil for empty collections.

What leads you to expect both to return nil?

Comment by Alex Miller [ 09/Oct/14 10:12 AM ]

Working as expected per docs.

seq -> returns a sequence
lazy-seq -> returns a seqable (when seq is called on it, will return a sequence)

Comment by Jeremy Betts [ 09/Oct/14 10:38 AM ]

I listed as enhancement and not defect based on documentation.
lazy-seq:
"Takes a body of expressions that returns an ISeq or nil, and yields
a Seqable object that will invoke the body only the first time seq
is called, and will cache the result and return it on all subsequent
seq calls. See also - realized?"

it's not clearly stated what it should return for [] where as for seq, it clearly stated that it will return nil for [].

given the intent of lazy-seq is to make the same result as seq, except that its members are evaluated when called for and not upfront.

The current implementation forces code to be aware of if it's dealing with lazy or non lazy sequences. This is not ideal.

Once again, listed as feature enhancement, because of the less than ideal design and documentation. Listed as minor, as it's fairly easy to work around it.

-Jeremy

Comment by Jeremy Betts [ 09/Oct/14 10:41 AM ]

how do i reopen this as the answers to this are not well thought out.

Comment by Alex Miller [ 09/Oct/14 11:57 AM ]

Despite the similarity in naming, seq and lazy-seq have different purposes which I believe are adequately stated in their doc strings. Of particular note, the intent of lazy-seq is NOT to make a seq, but to make a seqable (conceptually similar to the difference between Iterator and Iterable in Java).

Sequences are a logical list. Empty sequences are represented by nil. seq produces a sequence from the input. Because it produces either nil or a sequence with at least one value, seq is often used in a condition or termination check when walking through a sequence.

lazy-seq is a tool that can be used to create lazy sequences from a function, but it delays that computation by returning a seqable (not a seq) so that computation will only be forced at the point where you start producing a seq from it.

Most sequence functions implicitly call seq on their input (thus producing seqs from seqables), so the difference between them can often be missed.

[] is a seqable. lazy-seq will itself call seq on the result of the generator function if it is not a lazy seq. So, you give [] to lazy-seq and it creates a seqable with a function that returns []. When you call seq on the seqable, the function is evaluated to a PersistentVector (not a lazy seq) and then seq is called on it, which produces a nil, which is returned.

I do not see how this affects callers. Because sequence functions implicitly call seq, all of the sequence functions will work with either and yield the same results. Explicit use of lazy-seq is relatively rare (it's most commonly used when a sequence is produced by repeated evaluation of a function).

You might find this page to be helpful: http://clojure.org/sequences

Comment by Jeremy Betts [ 09/Oct/14 1:06 PM ]

(if (lazy-seq []) "yes" "no")
=> "yes"
(if (seq []) "yes" "no")
=> "no"

The truth value of an empty seq and an empty lazy-seq is different.

I guess i'm still not understand why this would be a "bad" change to the behavior?

Comment by Andy Fingerhut [ 09/Oct/14 1:15 PM ]

Jeremy, a lot of Clojure code uses the return value of seq in the way you show in your example to decide whether there is more to a sequence to process.

I have never seen Clojure code use lazy-seq for any purpose other than to construct a lazy sequence and return it from a recursive function, to avoid blowing the stack.

(lazy-seq x) returns a truthy value for all values of x, even nil.

Comment by Jozef Wagner [ 09/Oct/14 1:30 PM ]

Jeremy, have look on a sequence if you do not want nil returned. And Andy had a good point. lazy-seq is a constructor of LazySeq type, and as such should not return nil. seq returns either nil or an object that implements ISeq interface, but does not prescribe any concrete type. the nil return value from seq is a feature and is a one of differences between how seq and sequence behaves.

Comment by Jeremy Betts [ 09/Oct/14 2:16 PM ]

Jozef Wagner's answer is good. 'sequence' is the equivalent to 'lazy-seq' This was a problem for me as the laziness of something could not be changed without the inadvertent changing of behavior. This is what i ran into and why i entered the issue.

I'd still argue that a document update would would be a good thing.

-Jeremy





[CLJ-1557] Nested reduced is broken Created: 09/Oct/14  Updated: 10/Oct/14  Resolved: 10/Oct/14

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File re-reduced.diff    
Patch: Code
Approval: Vetted

 Description   

Re-reduced from composed transformation functions:

  • re-wraps the Reduced when it should not (take)
  • forget to unwrap the Reduced (partition-by, partition-all)
; nested reduced
=> (transduce (comp (take 1)) conj [:a])
[:a]
=> (transduce (comp (take 1) (take 1)) conj [:a])
#<Reduced@65979031: [:a]>
=> (transduce (comp (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@fcbc8d1: #<Reduced@60bea99a: [:a]>>
=> (transduce (comp (take 1) (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@6e9915bb: #<Reduced@5c712302: #<Reduced@472b9f70: [:a]>>>
 
; problems not appearing in all contexts
; not ok with transduce
=> (transduce (comp (partition-by keyword?) (take 1)) conj [] [:a])
#<Reduced@5156c42e: [[:a]]>
; but ok with sequence
=> (sequence (comp (partition-by keyword?) (take 1)) [:a])
([:a])
; well, not always
=> (sequence (comp (partition-by keyword?) (take 1)  (partition-by keyword?) (take 1)) [:a])
ClassCastException clojure.lang.Reduced cannot be cast to clojure.lang.LazyTransformer  clojure.lang.LazyTransformer$Stepper$1.invoke (LazyTransformer.java:104)

See also: https://groups.google.com/d/msg/clojure-dev/cWzMS_qqgcM/7IAhzMKzVigJ



 Comments   
Comment by Ghadi Shayban [ 09/Oct/14 11:11 PM ]

Same with partition-all

(transduce (comp (take 1) (partition-all 3) (take 1)) conj [] (range 15))
 #<Reduced@84f8976: [[0]]>
Comment by Christophe Grand [ 10/Oct/14 5:50 AM ]

patch for take, partition-by and partition-all





[CLJ-1555] Set literal duplicate check not consistent with set semantics Created: 09/Oct/14  Updated: 09/Oct/14  Resolved: 09/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

Having two functions with the same definition in a set literal signals an error:

#{(fn []) (fn [])}
; IllegalArgumentException Duplicate key: (fn [])

On the other hand, even though the definitions are the same, it still are two different functions that aren't equal.

(= (fn []) (fn []))
;=> false
(conj #{} (fn []) (fn []))
;=> #{#<user$eval14553$fn__14556 user$eval14553$fn__14556@5f04ed52> #<user$eval14553$fn__14554 user$eval14553$fn__14554@6f3d47f5>}

Therefore, the set literal above should not complain about duplicate keys.



 Comments   
Comment by Jozef Wagner [ 09/Oct/14 7:30 AM ]

duplicate of CLJ-1538





[CLJ-1549] split IReduce Created: 06/Oct/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1549-2.patch     Text File clj-1549.patch    
Patch: Code
Approval: Ok

 Description   
  • IReduceInit should take arity-2 version from existing IReduce
  • IReduce should extend IReduceInit and add arity-1
  • new stuff should implement IReduceInit only (audit everything added for 1.7)
  • old stuff should not change or break

Patch: clj-1549-2.patch



 Comments   
Comment by Alex Miller [ 06/Oct/14 4:56 PM ]

Patch does as requested. Did not change the CollReduce extension which currently needs both arities:

(extend-protocol CollReduce
  ...

  clojure.lang.IReduce
  (coll-reduce
   ([coll f] (.reduce coll f))
   ([coll f val] (.reduce coll f val)))
Comment by Rich Hickey [ 07/Oct/14 8:29 AM ]

Can we please use the name IReduceInit instead of ILeftReduce?





[CLJ-1547] range cause OutOfMemoryError when start > end AND step is zero Created: 04/Oct/14  Updated: 04/Oct/14  Resolved: 04/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Édipo L Féderle Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Mac OSX 10.9.4
java version "1.7.0_51"
Java(TM) SE Runtime Environment (build 1.7.0_51-b13)
Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode)



 Description   

I am playing with range function and I tried the following args:

(range 10 4 0) ;=> OutOfMemoryError

I add a test on range code to check for this condition, but probably I dont do it right, one test fail and I dont know why. If this is really a bug and someone can help me with this patch I appreciate it.



 Comments   
Comment by Nicola Mometto [ 04/Oct/14 8:43 AM ]

Not a bug, the docstring is explicit about this: "When step is equal to 0, returns an infinite sequence of start."

Comment by Édipo L Féderle [ 04/Oct/14 8:49 AM ]

Oh yeah, my fault. I will close this "issue".

Thanks Nicola.

Comment by Édipo L Féderle [ 04/Oct/14 8:49 AM ]

not a bug





[CLJ-1541] System/getProperty "user.dir" gives wrong output Created: 30/Sep/14  Updated: 30/Sep/14  Resolved: 30/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Khuram U. Khalid Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: System/getProperty, bug, user.dir
Environment:

Windows 8.1 - Java version 1.8.0 - Clojure 1.6.0 - IntelliJ IDEA - Maven 3.0.5



 Description   

;; For example if current project is in C:\Projects\My Project
;; Following gives...

(ns my.project.com.core)
(defn -main [] (println (System/getProperty "user.dir"))

;;=> C:\Projects\My Project\src\main\my\project\com

;; While when same Clojure code is run from a Java project gives...
public static void main(String[] args) { my.project.com.core.main(); }
;;=> C:\Projects\My Project

Expected same behavior and hence correct output in Clojure.



 Comments   
Comment by Alex Miller [ 30/Sep/14 9:18 AM ]

I tried this on a simple project at the command line and saw no difference in behavior between Java and Clojure. Clojure does not modify the user.dir system property and you are calling directly into the System class, just like Java does, so the only difference is in how your environment is configured when running in these two contexts.

It's possible that your environment (IDEA) is configuring Java and Clojure projects differently, but you should ask on the mailing list or issue tracker for the tool.





[CLJ-1540] Make main function to run when using on the fly compilation, not just ahead-of-time compilation Created: 29/Sep/14  Updated: 29/Sep/14  Resolved: 29/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: macdevign Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

From this doc
http://clojure.org/compilation

Clojure compiles all code you load on-the-fly into JVM bytecode, but sometimes it is advantageous to compile ahead-of-time (AOT). Some reasons to use AOT compilation are:
To generate named classes for use by Java among the reason

and only named classes can run off main function.

So if not using AOT, the main method will not be executed.

Hence the following main can only run in AOT using named classes.
(defn -main
(println "runme")))

Will that be possible to run the main function using on the fly compilation ?

Basically, it should work similarly to Java. If the clojure file has a main function then it should run the file if user select it to run (eg in IDE) regardless of mode of compilation.

For example, in IntelliJ ide, a clojure file (eg hello.clj) has the following code

(defn testme[]
(println "hello"))

(defn -main
(println "runme")
(testme))

if user choose "Run hello.clj" from Intellij, then it should execute the main function.

Will be great if this feature is consider in next release of clojure

thank



 Comments   
Comment by Alex Miller [ 29/Sep/14 12:49 PM ]

There are (already) a variety of ways to start a Clojure script or program and I believe what you request (and more) is already possible.

See: http://clojure.org/repl_and_main

An example command-line for your hello.clj example would be:

java -cp clojure.jar clojure.main -i hello.clj -e "(-main)"

but if you are only running this as a script you could embed the code to run your app at the end of the hello.clj script file and do:

java -cp clojure.jar clojure.main hello.clj
Comment by Kevin Downey [ 29/Sep/14 3:44 PM ]

checkout the `-m` option





[CLJ-1539] Allow Records to be imported "Normally" Created: 28/Sep/14  Updated: 28/Sep/14  Resolved: 28/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: David Williams Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I know about records, and how they are compiled to Java classes, etc. The thing is, the import of a record type has an undocumented quirk, the need to turn dashes into underscore

(:require [my-fancy.namespace])
(:import [my_fancy.namespace MyRecord])

Granted this is trivial, but I just spent an hour or two tracking this down after some initial unsuccessful attempts to import a record between namespaces. IMHO this is not user friendly and could be smoothed out.



 Comments   
Comment by Alex Miller [ 28/Sep/14 10:42 PM ]

There are no plans to change this. Typically you don't need to import the record class at all, just require the ns and use the > and map> constructor functions. When you do import the class, you are doing so as a Java class, so it follows java class import rules.





[CLJ-1537] Audit IReduce usages for proper Reduced handling Created: 26/Sep/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File audit-ireduce.diff     File clj-1537-v2.diff     File clj-1537-v3.diff    
Patch: Code
Approval: Ok

 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



 Comments   
Comment by Alex Miller [ 30/Sep/14 5:59 PM ]

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

Comment by Alex Miller [ 30/Sep/14 8:25 PM ]

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?

Comment by Timothy Baldridge [ 03/Oct/14 10:18 AM ]

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

Comment by Timothy Baldridge [ 03/Oct/14 10:29 AM ]

Failure examples from master, are added to the description.

Comment by Timothy Baldridge [ 03/Oct/14 2:23 PM ]

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





[CLJ-1535] Make boxed math warning suppressible Created: 26/Sep/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: math

Attachments: Text File clj-1535-2.patch     Text File clj-1535-3.patch     Text File clj-1535.patch     Text File silence-boxed-patch-10-01-2014.patch    
Patch: Code and Test
Approval: Ok

 Description   

Clojure 1.7.0-alpha2 included a new warning that will notify on use of boxed math when unchecked-math is set to true (CLJ-1325). Based on feedback, would like to make these warnings optional.

Approach: Revert (set! *unchecked-math* true) to prior behavior. Only emit warnings when (set! *unchecked-math* :warn-on-boxed).

Patch: clj-1535-3.patch

Screened by: Stuart Halloway



 Comments   
Comment by Michael Blume [ 01/Oct/14 7:45 PM ]

So I decided to take a shot at writing a patch for this. This is my first Clojure core patch, so I've probably messed up some formatting, but the implementation was pretty simple and the tests pass.

I introduced a variable, clojure.core/silence-boxed which defaults false and, when true, silences boxed math warnings. If the reverse is preferred (warn-boxed or similar) I can do that too.

Comment by Alex Miller [ 01/Oct/14 8:34 PM ]

Hi Michael, we have other plans for how this should be implemented, so will likely not use your patch. In the future, it's always good to check if the ticket is already assigned to someone before working on it.

Comment by Alex Miller [ 07/Oct/14 8:12 AM ]

Added clj-1535-3.patch, which is exactly the same diff as clj-1535-2.patch, but just squashes into a single commit.





[CLJ-1531] inc always warns when *unchecked-math* is set Created: 23/Sep/14  Updated: 23/Sep/14  Resolved: 23/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Pierre-Yves Ritschard
Resolution: Not Reproducible Votes: 0
Labels: bug, errormsgs


 Description   

While testing 1.7-alpha2 I stumbled this (affects clojure.data.codec amongst others). inc inlines a call to clojure.lang.Numbers's inc method which according to the rules of CLJ-1325 will warn.

I can't find a way around it for now, except maybe having a primitive-inc and primitive-dec java method which would be inlined in that case.

Happy to work on a patch but would prefer discussing it first.



 Comments   
Comment by Nicola Mometto [ 23/Sep/14 12:42 PM ]

I cannot reproduce this:

Clojure 1.7.0-master-SNAPSHOT
user=> (set! *unchecked-math* true)
true
user=> (inc 1)
2

Looking at Numbers.java I see both unchecked_inc and inc have long/double taking methods.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:49 PM ]

you're right, i must have been confused.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:50 PM ]

not a bug





[CLJ-1525] bean function returns mutable maps Created: 16/Sep/14  Updated: 22/Sep/14  Resolved: 22/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Simone Mosciatti Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux



 Description   

Please take a look at this snippet.

user> (import 'java.util.Date)
java.util.Date
user> (def now (Date.))
#'user/now
user> now
#inst "2014-09-17T03:14:13.821-00:00"
user> (def bean-map (bean now))
#'user/bean-map
user> bean-map
{:day 3, :date 17, :time 1410923653821, :month 8, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -120, :hours 5, :minutes 14}
user> (.setMonth now 1)
nil
user> bean-map
{:day 1, :date 17, :time 1392610453821, :month 1, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -60, :hours 5, :minutes 14}

The same snippet here. https://gist.github.com/siscia/032bff669bbc6fb0fe57



 Comments   
Comment by Jozef Wagner [ 17/Sep/14 1:32 AM ]

It works as expected. bean fn returns a clojuresque abstraction on top of live bean. map-like abstraction returned from bean is intended to be 'mutable' in sense that it always return the latest value. Otherwise it is read only.

Comment by Simone Mosciatti [ 17/Sep/14 1:42 AM ]

Hi,

sorry, the documentation didn't mention the "mutable" part so I was expecting an immutable map as always.

Sorry, about that.

Greets

Comment by Alex Miller [ 22/Sep/14 9:38 AM ]

this is the expected behavior





[CLJ-1524] SeqIterator constructor change broke binary compatibility in 1.7.0-alpha2 Created: 09/Sep/14  Updated: 09/Sep/14  Resolved: 09/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1524.diff    
Patch: Code
Approval: Ok

 Description   

Running code AOT-compiled against Clojure 1.6.0 (or older) with 1.7.0-alpha2 runtime will encounter this error with SeqIterator:

CompilerException java.lang.NoSuchMethodError: clojure.lang.SeqIterator.<init>(Lclojure/lang/ISeq;)V, compiling:(form-init5913779045640355531.clj:1:11)

Cause: This is due to a type change in the constructor of SeqIterator from ISeq to Object (commit: https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c ).

Proposed: Add the ISeq constructor back so that calls into that constructor retain backwards binary compatibility.

Patch: clj-1524.diff

Screened by:

More: From Datomic mailing list - https://groups.google.com/forum/#!topic/datomic/KZqhY6hUHz0



 Comments   
Comment by Alex Miller [ 09/Sep/14 11:06 AM ]

Patch not applied, but similar change applied directly here:

https://github.com/clojure/clojure/commit/ba41f25b6f3f32729c55f7f7ceb179be597acf94





[CLJ-1518] Patch for removing transient thread owner check broke rrb-vector Created: 03/Sep/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1518.diff    
Patch: Code
Approval: Ok

 Description   

The patch for CLJ-1498 changed the field types inside the persistent data structures, which inadvertently broke core.rrb-vector, which relies on reusing some of those internals. It is not necessary to change the type to satisfy the patch, so we would like to rollback that aspect of the change to minimize breakage.



 Comments   
Comment by Alex Miller [ 03/Sep/14 2:05 PM ]

In the patch I rolled back the changes in the Persistent*.java from CLJ-1498 and re-applied. The only "real" changes after the rollback are in ensureEditable(). Tests were left of course.





[CLJ-1512] Create volatile box for managing state Created: 25/Aug/14  Updated: 07/Oct/14  Resolved: 03/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File volatile2.diff     File volatile3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Motivation:

Clojure needs a faster variant of Atom for managing state inside transducers. That is, Atoms do the job, but they provide a little too much capability for the purposes of transducers. Specifically the compare and swap semantics of Atoms add too much overhead. Therefore, it was determined that a simple volatile ref type would work to ensure basic propagation of its value to other threads and reads of the latest write from any other thread. While updates are subject to race conditions, access is controlled by JVM guarantees.

Solution overview: Create a concrete type in Java, akin to clojure.lang.Box, but volatile inside supports IDeref, but not watches etc.

API:

(volatile! x) ;;ctor
(vreset! vol newval) ;;like reset
(vswap! vol f args) ;;same shape as swap!, but MACRO over vreset!

Patch: volatile3.diff

Screened by: fogus



 Comments   
Comment by Alex Miller [ 25/Aug/14 9:11 AM ]

Dumb benchmark before/after...

java -cp target/classes -Xmx512m -server clojure.main
(def t (take 1000000))
(def v (doall (range 1000000)))
(defn bench [t v]
  (time (into [] t v)))
(dotimes [_ 30] (bench t v))

before - 29-32 ms after warmup
after - 22-23 ms after warmup

Comment by Alex Miller [ 25/Aug/14 9:12 AM ]

From Stu H elsewhere:

Three questions:
1) Should we keep volatile? in the public API?
2) Should we work in terms of IVolatile interface (guessing no)
3) Do we need a CLJS version of these APIs?

Comment by Alex Miller [ 25/Aug/14 9:13 AM ]

1. We have many tickets requesting predicates over types that are "internal" and generally I find these to be helpful. They also can help in making core more portable to cljs (maybe those fns would fall back to atoms in cljs?).
2. We have tickets requesting the equivalent of this for IAtom (CLJ-803) etc. I don't think an interface adds any value to us here though. There seems to be some requests for this kind of passthrough interface from tooling as a decoupling point. Not putting my finger on those discussions but I know I've heard this, maybe on the mailing list.
3. I think yes if that allows us to be more efficient than whatever is being done now. Not obvious to me.

Comment by Nicola Mometto [ 25/Aug/14 9:40 AM ]

Why is vswap! a macro?

Comment by Ambrose Bonnaire-Sergeant [ 26/Aug/14 8:04 AM ]

An IAtom conversation: https://groups.google.com/forum/#!searchin/clojure-dev/iatom/clojure-dev/y5QoMqd44Lc/y4YmW09blk0J

Comment by Max Penet [ 26/Aug/14 10:28 AM ]

the vswap! macro is probably for performance reasons (the main motivation of this code to begin with), to avoid using apply or unrolling tons of arities

Comment by Nicola Mometto [ 26/Aug/14 1:07 PM ]

If that is the only reason, why can't it be a regular fn + :inline metadata?

Comment by Jozef Wagner [ 27/Aug/14 3:50 AM ]

why the bang in the name of volatile! function? If the reason is to warn users that this is an 'expert only' stuff, I suggest to use a verbose name instead, e.g. volatile-reference. (This will also be consistent with approach chosen in the names of volatile-mutable and unsynchronized-mutable hints.)

Comment by Rich Hickey [ 27/Aug/14 6:37 AM ]

Can you please lift the with-meta stuff out of the syntax-quote?
Actually, if volatile! ctor returned a type-hinted value that extra hinting might not even be needed. Let's do both for now.

Also the type hint on the volatile? arg makes no sense - it's a predicate asking if something is a volatile.

Comment by Alex Miller [ 28/Aug/14 9:05 AM ]

Made changes as requested.

Comment by Fogus [ 29/Aug/14 11:01 AM ]

I downloaded the patch and applied to latest master. I ran the isolated tests and the full test suite and also ensured that the patch didn't add any reflection warnings. I then modified the ticket description to add a little more context and motivation (for future readers). The code is straight-forward and clean.

Comment by Alex Miller [ 29/Aug/14 4:31 PM ]

Updated to volatile3.diff to address offline comment from Rich.





[CLJ-1511] stack overflow when comparing sequence results Created: 24/Aug/14  Updated: 07/Oct/14  Resolved: 27/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

OS X 10.9.4


Attachments: Text File 0001-provide-working-implementations-for-LazyTransform-eq.patch    
Patch: Code and Test
Approval: Ok

 Description   

Comparing sequences created with sequence causes a stack overflow when used as first argument to =.

Consider this transducer:

user=> (def map-inc (map inc))
#'user/map-inc

When creating a sequence and comparing with expected results, it works fine as the second argument to the comparison:

user=> (= (range 1 11) (sequence map-inc (range 10)))
true

But a stack overflow occurs when the order of arguments is reversed:

user=> (= (sequence map-inc (range 10)) (range 1 11))

StackOverflowError   clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
user=> (clojure.stacktrace/print-stack-trace *e 10)
java.lang.StackOverflowError: null
 at clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
nil

The error persists, even if the sequence is forced with doall:

user=> (= (doall (sequence map-inc (range 10))) (doall (range 1 11)))

StackOverflowError   clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)

It does work as expected, however, if the sequence is converted to a vector:

user=> (= (vec (sequence map-inc (range 10))) (range 1 11))
true


 Comments   
Comment by Nicola Mometto [ 25/Aug/14 4:31 AM ]

Patch provides equiv/equals implementations for LazyTransform based on ASeq equiv/equals





[CLJ-1510] line-seq and read-line don't return nil on Ctrl-D in lein repl Created: 21/Aug/14  Updated: 25/Aug/14  Resolved: 22/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Geoff Little Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: reader
Environment:

OSX, lein repl



 Description   

Executing in lein repl either

(doseq [line (line-seq (java.io.BufferedReader. *in*)) :while line]
(println line))

Or

(doseq [line (read-line) :while line]
(println line))

One would expect these to return on a user's enter of Ctrl-D, however they never return.



 Comments   
Comment by Andy Fingerhut [ 21/Aug/14 11:51 PM ]

If instead of 'lein repl' you run:

java -cp clojure.jar clojure.main

at a shell prompt, you get a REPL where the first doseq expression you give does return when you enter Ctrl-D.

The second doseq expression returns a string from (read-line), and then the doseq iterates over that as a sequence of characters, and it returns without needing Ctrl-D after reading a single line in both 'lein repl' and the plain Clojure REPL with the command given above.

I suggest that the behavior with your first doseq expression is an issue to be taken up with the Leiningen developers. Several ways of contacting them are given at http://leiningen.org/#community I would recommend IRC or the email list first, before creating a Github issue.

Comment by Alex Miller [ 22/Aug/14 8:29 AM ]

Agreed with Andy - these examples work fine in base repl but other environments may be affecting your input upstream of clojure.

Comment by Andy Fingerhut [ 25/Aug/14 11:42 AM ]

Geoff, it looks like there have been recent changes committed to the tools.nrepl library related to this issue. tools.nrepl is used within Leiningen to implements its REPL.

https://github.com/clojure/tools.nrepl/commit/eb526fd8498ced1b4bd1555f8ff680f3ad65f1b4





[CLJ-1505] Sorry I have to enter this test bug Created: 15/Aug/14  Updated: 15/Aug/14  Resolved: 15/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Karen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Interacting with this site caused FireFox to freak out when populating a field in our app so I need to enter a bug here to figure out why. See http://dev.clojure.org/jira/browse/CLJ-1378 We get the same error message in one of our summary field. It is a FireFox bug so I'll update this when I am done. Apologies.






[CLJ-1503] allow for `{~@foo} and `#{~(gensym) ~(gensym)} Created: 14/Aug/14  Updated: 14/Oct/14  Resolved: 24/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File 0001-allow-for-foo-and-gensym-gensym.patch    
Patch: Code

 Description   

Currently both `{@foo} and `#{(gensym) ~(gensym)} throw an exception at read time even though they could actually return valid run-time code.
This patch introduces the SyntaxQuotedMap and SyntaxQuotedSet classes that are used internally in the reader to represent syntax quoted maps and sets, that may skip the duplicate key and length checks.

The SyntaxQuotedMap extends PersistentArrayMap as a workaround for the lack of defrecords in java, since a SyntaxQuotedMap needs to be an IPersistentMap in case it's used as metadata.



 Comments   
Comment by Nicola Mometto [ 24/Sep/14 5:04 AM ]

Duplicated of http://dev.clojure.org/jira/browse/CLJ-1425
Also I no longer think this is a good idea

Comment by Kevin Downey [ 14/Oct/14 1:27 PM ]

It seems like this is a bad idea, it sort of makes sense from purely a macro writing perspective, but syntax quote is used outside of macros, in which case this just becomes a circumvention of the duplicate key checks that were added, I think some time around 1.3 maybe 1.4





[CLJ-1500] Cache namespace env Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

See: https://github.com/clojure/tools.analyzer.js/blob/master/src/main/clojure/clojure/tools/analyzer/js.clj#L524-L548






[CLJ-1498] Remove birth-thread check from transients Created: 08/Aug/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: collections, transient

Attachments: File clj-1498-2.diff     File clj-1498-3.diff     File clj-1498.diff    
Patch: Code
Approval: Ok

 Description   

Transients protect themselves from use by any thread other than the one that creates them. This is good for safety, however it eliminates certain valid usages of transients. For example, usage in a go-block might occur in subsequent invocations across multiple OS threads (but only one logical thread of control).

Current simple test:

user> (def v (transient []))
#'user/v
user> (persistent! @(future (conj! v 1)))
IllegalAccessError Transient used by non-owner thread  clojure.lang.PersistentVector$TransientVector.ensureEditable (PersistentVector.java:464)

Proposal: Remove the owner check from transient collections. (Leave the edit after persistent check as is.) The test above should succeed.

After:

user=> (def v (transient []))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

The clj-1498-3.diff version of the patch also replaces the AtomicReference<Thread> with AtomicBoolean as we can now track just ownership, not who owns it.

Doc update: Various pieces of documentation will need to be updated with this change, namely http://clojure.org/transients

Patch: clj-1498-3.diff

Alternative: Another idea would be to make this check optional with some kind of option on the transient call (transient coll :check-owner true). Not sure whether what the default would be for that.



 Comments   
Comment by Jozef Wagner [ 09/Aug/14 7:08 AM ]

I suggest to add a functionality to pass ownership of a transient to the different thread, or to release the ownership by passing nil.

user=> (def v (pass! (transient []) nil))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

pass! has to be called by current owner thread, or by any thread if the transient is currently released.

Comment by Alex Miller [ 13/Aug/14 1:42 PM ]

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

Comment by Stuart Halloway [ 19/Aug/14 11:05 AM ]

Alex, can you please expand the example test you provided to a generative test that covers the following combinations:

  1. different collection sizes (above and below the ArrayMap size boundary)
  2. different shapes (vector vs. map)
  3. successful use across threads (positive use case this ticket enables)

data_structures.clj has helpers for generating transient interactions that you can build on.

Comment by Alex Miller [ 20/Aug/14 8:59 AM ]

Enhanced existing generative tests to test random actions against sets, vectors, and both PHM and PAM. Added additional actions to do transient modification actions in other threads as well as originating thread.





[CLJ-1497] sequence with transducers realizes n+2 elements Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1497.diff     File clj-1497v2.diff    
Approval: Ok

 Description   

The first element is realized at creation time:

user=> (def a (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
#'user/a

Fully realizing the sequence realizes the other n-1 elements, and 2 more:

user=> a
(~2
~3
1 ~4
2)

Compare with version using seq operations:

user=> (def a (sequence (take 2 (map #(do (println (str "~" %)) %) (iterate inc 1)))))
#'user/a
user=> a
(~1
~2
1 2)

Transduce also doesn't seem to exhibit this issue:

user=> (def a (transduce (take 2) conj [] (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
~2
#'user/a
user=> a
[1 2]


 Comments   
Comment by Alex Miller [ 08/Aug/14 10:02 AM ]

Patch attached that improves the issue - will now only realize n+1 elements.

Comment by Nicola Mometto [ 08/Aug/14 10:16 AM ]

Nice, I added a commit on top of yours to delay the realization of the first element of the lazyseq to the first .next call instead of on SeqIteration creation

Comment by Alex Miller [ 08/Aug/14 11:12 AM ]

Fixed by Rich directly, not by patch.





[CLJ-1494] remove flatmap in favor of mapcat Created: 07/Aug/14  Updated: 03/Sep/14  Resolved: 03/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-remove-flatmap-use-1-arity-mapcat-instead.patch    
Patch: Code

 Description   

While all the transducers functions are implemented as an arity in the matching clojure core sequence, for mapcat a new function has been added: flatmap.
The reason for this is, as Rich said in a HN comment, "because mapcat's signature was not amenable to the additional arity".
This patch changes the mapcat signature to take at least one collection so that it's possible to add the 1-arity for the transducer function, eliminating the need for a different function, flatmap.

There has been no loss by removing the 1-arity version of mapcat as a sequence function since trying to use (mapcat f) as currently defined (not as redefined with this patch) would fail before transducers, and after transducers:
Before transducers (mapcat f) would result in a call to (map f) which would fail with an ArityException
After transducers that (map f) call would return a function, which then would be used as an argument to (apply concat the-f), resulting in a IllegalArgumentException since apply expects a sequence but it's been given a fn.



 Comments   
Comment by Alex Miller [ 03/Sep/14 11:02 AM ]

Done as a result of https://github.com/clojure/clojure/commit/7d84a9f6f35a503cddf98487b6544d18937c669e





[CLJ-1491] External type hint inconsistency between regular functions and primitive functions Created: 05/Aug/14  Updated: 25/Sep/14  Resolved: 25/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: compiler, typehints

Attachments: Text File 0001-preserve-fn-meta-on-invokePrim.patch    
Patch: Code
Approval: Triaged

 Description   

Consider the following example.

(set! *warn-on-reflection* true)

(defn f [n] (java.util.ArrayList. (int n)))

(let [al ^java.util.ArrayList (f 10)]
  (.add al 23))

As expected this does not warn about reflection. The following example shows the same scenario for a primitive function.

(set! *warn-on-reflection* true)

(defn g [^long n] (java.util.ArrayList. n))

(let [al ^java.util.ArrayList (g 10)]
  (.add al 23))
; Reflection warning, NO_SOURCE_PATH:2:3 - call to method add on java.lang.Object can't be resolved (no such method).

So the behavior of external type hints is inconsistent for regular functions and primitive functions.
Most likely, the external type hint information is somehow ignored for primitive functions since the case where they return no primitive value is not treated separately.



 Comments   
Comment by Nicola Mometto [ 05/Aug/14 4:32 AM ]

The following patch preserves the original metadata of the invoke form on the transformed .invokePrim expression

Comment by Alex Miller [ 05/Aug/14 7:40 AM ]

Not challenging the premise at all but workaround:

(let [^java.util.ArrayList al (g 10)]
  (.add al 23))
Comment by Gunnar Völkel [ 05/Aug/14 8:09 AM ]

Well, the example above was already changed such that you can also place the type hint on the binding to check whether that works.
The actual problem arose when using the return value of the function exactly once without an additional binding.

Comment by Jozef Wagner [ 05/Aug/14 10:48 AM ]

Responding to Alex's comment, is there a consensus on which variant is (more) idiomatic? IMHO latter variant seems to be more reliable (as this issue shows, and for primitive hits too), and is consistent with 'place hint on a symbol' idiom which is applied when type hinting vars or fn args.

(let [symbol ^typehint expr] body)
(let [^typehint symbol expr] body)
Comment by Alex Miller [ 05/Aug/14 4:59 PM ]

They have different meanings. Generally the latter covers some cases that the former does not so it's probably the better one. I believe one of the cases is that if expr is a macro, the typehint is lost in the former.

Comment by Nicola Mometto [ 25/Sep/14 9:59 AM ]

The patch for http://dev.clojure.org/jira/browse/CLJ-1533?jwupdated=61127&focusedCommentId=35814 fixes this issue and more and should be preferred over this

Comment by Alex Miller [ 25/Sep/14 10:31 AM ]

Dupe of CLJ-1533





[CLJ-1487] Variadic unrolling for partial Created: 01/Aug/14  Updated: 01/Aug/14  Resolved: 01/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File unroll-partial.patch    
Patch: Code and Test

 Description   

Many of the functions in clojure.core are variadic-unrolled for performance. The implementation of juxt, for example, is 29 lines where it could be just 3 if we didn't care about performance. For the function "partial", this unrolling is, if you will excuse the pun, only partially done. This patch extends the unrolling for partial to mirror that for juxt and comp, and includes a test that partial works for any reasonable number of arguments (the existing test just spot-checks a few arities).

I've done some performance benchmarking, and we get about a 20% speedup on calling partial functions, with no performance penalty for creating them.

You may note that the n-ary case, ([arg1 arg2 arg3 & more] ...), is not unrolled in my patch. Doing so would have worsened performance: because we're having to call apply anyway, there's no real benefit to be had from unrolling, and it costs a little to rebuild the arglist before passing it to apply.



 Comments   
Comment by Ghadi Shayban [ 01/Aug/14 6:45 PM ]

Dupe of CLJ-1430

Comment by Alex Miller [ 01/Aug/14 11:10 PM ]

Dupe as noted in the comments





[CLJ-1481] Typo in type-reflect's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

membrer -> member

Screened by: Alex Miller






[CLJ-1480] Incorrect param name reference in defmulti's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-param-name-reference-in-defmulti-s-docstring.patch    
Patch: Code
Approval: Ok

 Description   

attribute-map should actually be attr-map

Screened by: Alex Miller






[CLJ-1479] Typo in filterv example Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

filter -> filterv in changes.md

Screened by: Alex Miller






[CLJ-1478] Doc typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Another small typo fix:
from from -> from

Screened by: Alex Miller






[CLJ-1477] Fixed a typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Just a simple typo fix - "directy" -> "directly".

Screened by: Alex Miller






[CLJ-1476] map-invert should use (empty m) instead of {} Created: 26/Jul/14  Updated: 27/Jul/14  Resolved: 27/Jul/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gregory Schlomoff Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

clojure.set/map-invert should reduce with (empty m) instead of {} so that it returns a map of the same type as its argument.

This is a trivial change and I'm willing to submit a patch if nobody opposes.



 Comments   
Comment by Alex Miller [ 26/Jul/14 8:43 AM ]

I don't think that always makes sense. Say you had a map of string to integers with a custom comparator created by sorted-map-by. If you use empty, you'd still have a map with a custom comparator which you would pour integer keys into and would likely throw a ClassCastException.

What is the use case that led you to this ticket?

Comment by Gregory Schlomoff [ 26/Jul/14 9:14 AM ]

Hello Alex, thanks for commenting.

My use case is that I have a custom type that implements IPersistentMap. If I use map-invert over it, I get a regular map back, which is problematic because regular maps don't allow multiple values for the same key, unlike my multimap implementation, so I loose information.

(map-invert (my-multimap :a 1, :b 1))
=> {1 :b} ; lost the (1 :a) entry because regular maps don't allow duplicate keys

Maybe a solution would be to make a version of map-invert that takes a map to insert the inverted entries into?

I'm not adamant over this, if you think there is no elegant solution for this issue we can close it.

Comment by Alex Miller [ 27/Jul/14 7:28 AM ]

I don't think this enhancement makes sense as written - there are cases where it would be a breaking change for existing code.

I do think your specified problem makes sense though. One enhancement might be to have a variant of map-invert (different arity or map-invert-into that took an additional map target param).





[CLJ-1474] `reduced` docstring should be more explicit Created: 25/Jul/14  Updated: 27/Jul/14  Resolved: 25/Jul/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: docstring


 Description   

The documentation for reduced is as follows:

Wraps x in a way such that a reduce will terminate with the value x

From what I gather, this does not specify whether the init value of a reduce could be a reduced value or not. As shown, the fact that the init value is a reduced value is ignored:

(reduce list (reduced 1) [2])
=> (#<Reduced@518a6aa: 1> 2)

The documentation should explicitly mention that a reduce call will not check if the initial value is reduced.



 Comments   
Comment by Alex Miller [ 25/Jul/14 9:09 AM ]

reduced creates a value that has special meaning as the output of invocation of the reducing function. Your example is about an input to that function. I don't see that this makes sense or needs documenting.

You can of course invent a situation where a (reduced 1) input is also the output but again, that seems like a pretty weird use case.

(reduce (fn [a v] a) (reduced 1) [2])
;; 1
Comment by Jean Niklas L'orange [ 25/Jul/14 12:10 PM ]

Right, that's my point. Nowhere in the documentation does it state that this does not apply to the initial value given to reduce. While you and I know this, I don't see how one can conclude this based on the current documentation.

Put differently, someone might wrongly assume that reduce is implemented as an optimised version of this:

(defn reduce [f init coll]
  (cond (reduced? init) (unreduced init)
        (empty? coll)    init
        :else           (recur f (f init (first coll))
                                 (rest coll))))

However, that's not the case, which I think is worth pointing out.

Comment by Alex Miller [ 25/Jul/14 5:01 PM ]

But it might apply to the initial value (as in my example where a reduced value is respected - note that doesn't return (reduced 1), just 1). Your suggested documentation change is talking about input values, but in my mind that leads to incorrect conclusions.

The only change that would make sense to me is clarifying where a "reduced" value is checked (on the result of applying the function passed to reduce). I think that's already implicit in the existing doc string myself. Since we have multiple implementations of "reduce", we have to tread carefully not to refer to explicitly to a particular one.

This use of a reduced initial value does not even make sense; why we would we confuse the docstring to warn about it?

Comment by Jean Niklas L'orange [ 27/Jul/14 7:20 AM ]

Ah, I get your point now, and I see how this would just create more confusion.

Thanks for the explanation.





[CLJ-1468] Add deep-merge and deep-merge-with Created: 18/Jul/14  Updated: 18/Jul/14  Resolved: 18/Jul/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: Text File CLJ-1468-deep-merge-01.patch    
Patch: Code and Test
Approval: Triaged

 Description   

When dealing with nested map structures, one often wants to merge two maps recursively.

The deep-merge-with function was originally written by Chris Houser for clojure.contrib.map-utils but was not maintained after clojure-contrib was split into separate modules.

deep-merge and deep-merge-with are widely copied, usually with the same implementation, in utility libraries. For example:



 Comments   
Comment by Rich Hickey [ 18/Jul/14 11:42 AM ]

Vague semantics and docs strings





[CLJ-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: ft, interop

Attachments: File iterable-bean-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   

The changes in Clojure 1.6 hashing revealed that `bean` does not return a map that implements Iterable:

user=> (hash (bean (java.util.Date.)))

AbstractMethodError clojure.lang.APersistentMap.iterator()Ljava/util/Iterator;  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.iterator (:-1)

Patch adds `iterator` method to clojure.core/bean.

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Jul/14 10:22 AM ]

One workaround:

(hash (apply hash-map (bean (java.util.Date.))))

Interestingly, into does not help b/c into uses reduce, which internally uses the iterator too.

Comment by Alex Miller [ 16/Jul/14 11:01 AM ]

APersistentMap implements Iterable and expects subclasses to fulfill that contract. The bean proxy does not. Instead of changing APersistentMap, why not add:

(iterator [] (.iterator pmap)

to the bean proxy definition?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:19 AM ]

It seemed like an oversight that APersistentMap lacked a default iterator method.

That said, I haven't used OO inheritance for 4 years. Should I change the patch?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:47 AM ]

Added new patch that just adds iterator to bean.





[CLJ-1465] SubVector leaks memory by design (?) Created: 14/Jul/14  Updated: 14/Jul/14  Resolved: 14/Jul/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Szymon Witamborski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: memory


 Description   

While reading through the code of SubVector class, I've noticed that the only thing that it does is creating a "subview" of an existing vector without doing anything to elements that are not accessible any more. Please correct me if I'm wrong but I think this leads to memory leaks in this scenario:

(let [a [1 2 3 4 5]
      b (subvec a 2 3)]
  b)
[3]

In this example we no longer have any reference to a once the (let...) expression returned. Elements of a that are no longer accessible will not be garbage collected until b is garbage collected because b still holds a reference to a (the v field in SubVector class). Ideally, these elements should be garbage collectible as soon as a is gone.



 Comments   
Comment by Alex Miller [ 14/Jul/14 2:40 PM ]

The subvec docstring says:

"Returns a persistent vector of the items in vector from start (inclusive) to end (exclusive). If end is not supplied, defaults to (count vector). This operation is O(1) and very fast, as the resulting vector shares structure with the original and no trimming is done."

The implementation is intentional to make this a constant-time operation. If you are willing to make the tradeoff re shared structure and object retention, this constant operation has better performance. In other words: working as intended.





[CLJ-1464] Incorrectly named parameter to fold function in reducers.clj Created: 12/Jul/14  Updated: 10/Sep/14  Resolved: 10/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 0001-Rename-a-function-parameter-to-reflect-the-fold-func.patch    
Patch: Code
Approval: Triaged

 Description   

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/reducers.clj#L95
The 2-arity fold accepts reducef as parameter and then uses it as a combinef.
Instead it should accept combinef as parameter and then use it as a reducef, as every combine fn (monoid) is a reduce fn, but not every reduce fn is a combine fn (it's not associative).



 Comments   
Comment by Jason Jackson [ 12/Jul/14 2:58 PM ]

this is my first patch for clojure please double check everything. CA is done.

Comment by Andy Fingerhut [ 12/Jul/14 7:29 PM ]

Everything gets double checked whether it is your first patch or your 50th

At least as far as the format of the patch being correct, that it applies cleanly to the latest version of Clojure on the master branch, compiles and passes all tests cleanly, all of that is good.

Whether there is interest in taking your proposed change is to be decided by others. It may be some time before it is examined further.

Comment by Jozef Wagner [ 13/Jul/14 4:11 AM ]

This is not a defect. Quoting Rich, "If no combining fn is supplied, the reducing fn is used." (source)

There are three user supplied operations in fold: getting identity element (combinef :: -> T), reducing function (reducef :: T * E -> T) and combining function (combinef :: T * T -> T). For reduce, combining function is not needed but the rest two operations are needed. Thus reducing function (reducef) supplies identity element for reducers and only in folders the identity element is produced by combining function. In case where reducing fn is used for both reducing and combining, it must of course be associative and must handle objects of types T and E as a second argument.

Comment by Jason Jackson [ 14/Jul/14 12:14 AM ]

@Jozef I appreciate the feedback I still think my patch is correct, although I admit everyone's time is better spent not debating this small refactoring so feel free to close it.

One perspective to view my patch from is if we had protocols p/Monoid and p/Reducer. It's possible to reify any object that implements p/Monoid into p/Reducer, but not the other way around since not every p/Reducer is associative.

Comment by Jason Jackson [ 14/Jul/14 12:30 AM ]

From that perspective you could also say that in the 2-arity case the parameter "reducef" requires objects that implement both p/Monoid and p/Reducer, but in the 3-arity case the parameter "reducef" only requires p/Reducer

Comment by Jozef Wagner [ 14/Jul/14 1:38 AM ]

Note that reducef and combinef take different type of second argument, so not every combining function can be used as a reducing one. Your proposal is thus no better than the status quo. Consider following example:

(fold clojure.set/union conj [1 1 2 2 3 3 4 4])




[CLJ-1450] Add a variant of range that's inclusive with respect to its upper boundary Created: 19/Jun/14  Updated: 19/Jun/14  Resolved: 19/Jun/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Such a variant of range might behave like this:

```
(irange 1 5)
;;=> (1 2 3 4 5)

(irange 1 2 5)
;;=> (1 3 5)
```

I'm suggesting this because often in practice we have to deal with inclusive ranges and having to add 1 to the upper boundary in such scenarios seems to reduce the clarity of the code. I guess something like this applies to some extend to lower boundary as well:

```
(erange 1 5)
;;=> (2 3 4)
```



 Comments   
Comment by Alex Miller [ 19/Jun/14 2:55 PM ]

We're not interested in adding these to core, thanks.





[CLJ-1448] Suggest alength in error message on attempt to access array length via .length Created: 19/Jun/14  Updated: 19/Jun/14  Resolved: 19/Jun/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Colin Taylor Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs

Attachments: Text File dot-length-recommend-alength.patch    
Patch: Code

 Description   

Problem:

Newcomers are easily confused by the inability to access <array>.length via (.length <array>).

Approach:

Append to invalid field access message, a suggestion to use alength for this specific case (class.isArray and field = "length")

user=> (.length (int-array 2))
IllegalArgumentException No matching field found: length for class [I, use alength function for array length


 Comments   
Comment by Alex Miller [ 19/Jun/14 2:56 PM ]

We don't have any plans to add anything this specific to the error check, thanks.





[CLJ-1440] Unable to exclude clojure.lang.Compiler using :refer-clojure Created: 06/Jun/14  Updated: 07/Jun/14  Resolved: 07/Jun/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler, interop


 Description   
(ns io.aviso.twixt.js-minification
  "Provides support for JavaScript minification using the Google Closure compiler."
  (:refer-clojure :exclude [Compiler])
  (:import (com.google.javascript.jscomp CompilerOptions ClosureCodingConvention DiagnosticGroups CheckLevel
                                         SourceFile Result Compiler))
  (:require [clojure.java.io :as io]
            [io.aviso.twixt.utils :as utils]
            [io.aviso.tracker :as t]
            [clojure.string :as str]))

Results in:

clojure.lang.Compiler$CompilerException: java.lang.IllegalStateException: Compiler already refers to: class clojure.lang.Compiler in namespace: io.aviso.twixt.js-minification, compiling:(/Users/hlship/workspaces/annadale/twixt/src/io/aviso/twixt/js_minification.clj:1:1)
        java.lang.IllegalStateException: Compiler already refers to: class clojure.lang.Compiler in namespace: io.aviso.twixt.js-minification
                                     clojure.lang.Namespace.referenceClass                    Namespace.java:  140
                                        clojure.lang.Namespace.importClass                    Namespace.java:  158
                                        clojure.lang.Namespace.importClass                    Namespace.java:  164
                   io.aviso.twixt.js-minification/eval4104/loading--auto--               js_minification.clj:    1
                                   io.aviso.twixt.js-minification/eval4104               js_minification.clj:    1
                                                clojure.lang.Compiler.eval                     Compiler.java: 6703
                                                clojure.lang.Compiler.eval                     Compiler.java: 6692
                                                clojure.lang.Compiler.load                     Compiler.java: 7130
                                   io.aviso.twixt.js-minification/eval4100  form-init4106199735960171933.clj:    1
                                                clojure.lang.Compiler.eval                     Compiler.java: 6703
                                                clojure.lang.Compiler.eval                     Compiler.java: 6666
                                                         clojure.core/eval                          core.clj: 2927
                                      clojure.main/repl/read-eval-print/fn                          main.clj:  239
                                         clojure.main/repl/read-eval-print                          main.clj:  239
                                                      clojure.main/repl/fn                          main.clj:  257
                                                         clojure.main/repl                          main.clj:  257
                                                clojure.lang.RestFn.invoke                       RestFn.java: 1096
             clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn            interruptible_eval.clj:   56
                                            clojure.lang.AFn.applyToHelper                          AFn.java:  152
                                                  clojure.lang.AFn.applyTo                          AFn.java:  144
                                                        clojure.core/apply                          core.clj:  624
                                               clojure.core/with-bindings*                          core.clj: 1862
                                                clojure.lang.RestFn.invoke                       RestFn.java:  425
                clojure.tools.nrepl.middleware.interruptible-eval/evaluate            interruptible_eval.clj:   41
clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval/fn/fn            interruptible_eval.clj:  171
                                                      clojure.core/comp/fn                          core.clj: 2402
             clojure.tools.nrepl.middleware.interruptible-eval/run-next/fn            interruptible_eval.clj:  138
                                                      clojure.lang.AFn.run                          AFn.java:   22
                         java.util.concurrent.ThreadPoolExecutor.runWorker           ThreadPoolExecutor.java: 1145
                        java.util.concurrent.ThreadPoolExecutor$Worker.run           ThreadPoolExecutor.java:  615
                                                      java.lang.Thread.run                       Thread.java:  724


 Comments   
Comment by Nicola Mometto [ 06/Jun/14 4:52 PM ]

refer and thus refer-clojure only works for Vars.
a workaround is:

(ns ..)
(ns-unmap *ns* 'Compiler)
(import 'com.google.javascript.jscomp.Compiler)
Comment by Alex Miller [ 07/Jun/14 11:34 AM ]

Ditto what Nicola said. Or just fully-qualify.





[CLJ-1439] Reduce keyword cache lookup cost Created: 05/Jun/14  Updated: 07/Oct/14  Resolved: 01/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: keywords, performance

Attachments: Text File 0001-Improve-Keyword.intern-performance.patch    
Patch: Code
Approval: Ok

 Description   

Background: Symbol is composed of name and namespace strings. Symbol construction interns both of these strings - this reduces memory usage and allows for string == checks inside Symbol. Keywords wrap a Symbol and have an additional cache to reuse Keyword instances.

Problem: Certain applications make heavy use of keywords (in particular the case of parsing or transforming JSON, XML, or other data into Clojure maps with keyword keys). Constructing the same keyword from a string over and over again will cause the string to be interned, a symbol constructed, and the lookup to occur in the keyword cache. In the case where the keyword already exists, this is more work than is necessary, making this path slower than it can be.

Reproduce: The following test simulates rounds of creating many keywords - the unique? flag indicates whether to use new or the same set of keywords each rep. unique?=false should be more similar to parsing a similar JSON record format over and over.

(set! *unchecked-math* true)

(defn kw-new [n unique?]
  (let [base (if unique? (str (rand)) "abcdef")]
    (loop [i 0
           kws (transient [])]
      (if (< i n)
        (recur (inc i) (conj! kws (keyword (str base i))))
        (persistent! kws)))))

(defn bench-kw [reps n unique?]
  (dotimes [_ reps]
    (let [begin (System/nanoTime)]
        (kw-new n unique?)
        (let [end (System/nanoTime)
              elapsed (/ (- end begin) 1000000.0)]
          (println elapsed "ms")))))

(bench-kw 50 10000 false)  ;; expected similar to JSON use case
(bench-kw 50 10000 true)   ;; for comparison

On 1.6, we see about 5.5 ms for repeated and 134 ms for unique after warmup.
With the patch, we see about 2.2 ms for repeated and 120 ms for unique after warmup.

Cause: Keyword construction based on a string involves:

  • Interning string(s) in new kw
  • Constructing Symbol with interned strings
  • Clearing Keywords from the Keyword cache if GC has reclaimed them
  • Constructing a new Keyword
  • Wrapping the Keyword in a WeakReference
  • CHM putIfAbsent on the cache
  • If new, return. If exists, get the old one and return.
  • In the event the Keyword is reclaimed by GC between the last 2 steps, retry.

This process involves a fair amount of speculative interning and object creation if the keyword already exist.

Proposal: Streamline the keyword construction process by reworking the cache implementation and the Keyword.intern() process. The patch changes the cache to key by string name instead of symbol, deferring interning and symbol creation on lookup to when we know the keyword construction is needed. The various Keyword.intern() methods are also reworked to take advantage if called with an existing Symbol to avoid re-creating it.

Patch: 0001-Improve-Keyword.intern-performance.patch

Related: CLJ-1415



 Comments   
Comment by Alex Miller [ 01/Aug/14 11:48 AM ]

Alternate changes were committed today to improve both symbol and keyword creation times.

https://github.com/clojure/clojure/commit/c9e70649d2652baf13b498c4c3ebb070118c4573

Comment by Alex Miller [ 07/Oct/14 4:42 PM ]

related patch was applied





[CLJ-1437] Keyword cache lookup could be faster Created: 05/Jun/14  Updated: 05/Jun/14  Resolved: 05/Jun/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: keywords, performance


 Description   

Breaking this piece out from CLJ-1415. Keyword cache lookup includes symbol creation and interning which could be avoided, making cache lookup faster.






[CLJ-1430] Improve performance of partial Created: 23/May/14  Updated: 05/Sep/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: performance

Attachments: File partial-perf.diff    
Patch: Code
Approval: Ok

 Description   

This patch improves performance of partial by only using apply when needed. The code structure follows that of juxt.

Performance benchmark:

(ns partial-test.core
  (:require [criterium.core :refer [bench]])
  (:gen-class))

(defn -main []
  (let [f (partial + 1 1)]
    (println "Starting")
    (bench (f 1 1))
    (println "Done")))

Results for 1.6.0:

Evaluation count : 228751140 in 60 samples of 3812519 calls.
             Execution time mean : 266.700063 ns
    Execution time std-deviation : 2.966851 ns
   Execution time lower quantile : 262.641023 ns ( 2.5%)
   Execution time upper quantile : 274.207916 ns (97.5%)
                   Overhead used : 1.610513 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

Results for 1.7.0 with this patch:

 Evaluation count : 348208140 in 60 samples of 5803469 calls.
              Execution time mean : 171.210533 ns
     Execution time std-deviation : 2.011660 ns
    Execution time lower quantile : 168.819526 ns ( 2.5%)
    Execution time upper quantile : 176.015584 ns (97.5%)
                    Overhead used : 2.644128 ns

 Found 3 outliers in 60 samples (5.0000 %)
 	low-severe	 3 (5.0000 %)
  Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

Benchmarks performed via lein uberjar + running via the commandline.

Patch: partial-perf.diff

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/May/14 10:46 AM ]

Screened, looks as expected.

Comment by Andy Fingerhut [ 02/Jun/14 10:50 AM ]

Timothy, just a nit that I would not have noticed except for my program that checks for name and email address of patch authors, to see if they are on my contributor's list, but do you really have both of the email addresses tbaldridge@gmail.com and tbaldidge@gmail.com (note the spelling difference)? The latter is the one on this patch.

Comment by Timothy Baldridge [ 02/Jun/14 11:04 AM ]

fixed email

Comment by Timothy Baldridge [ 02/Jun/14 11:05 AM ]

nice catch! it was a typeo in my .gitconfig defaults. I've fixed the patch.

Comment by Alex Miller [ 02/Jun/14 11:19 AM ]

Tim (and anyone really) - please let someone know if you need to change a screened patch! Looks fine here, but screener should be notified so they can re-screen.

Comment by Alex Baranosky [ 05/Sep/14 9:11 PM ]

Very nice patch. I've gotten into the habit of not using partial anymore for performance sensitive code. Perhaps this change means I need to rethink that.





[CLJ-1429] Cache unknown multimethod value default dispatch Created: 22/May/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: performance

Attachments: Text File clj-1429.patch    
Patch: Code
Approval: Ok

 Description   

Multimethods maintain a cache from dispatch value (result of the dispatch function) to dispatch method. If the dispatch value does not find a match in the available methods, it falls through to a lookup using the default dispatch value and returns that method. This default dispatch case is NOT recorded in the cache. This means that every case that falls through to the default case incurs a scan of the methodTable (and the class inheritance checks that involves).

Perf test:

(defmulti mm class)
(defmethod mm String [s] s)
(defmethod mm Long [l] l)
(defmethod mm :default [v] v)

(defn perf [reps size]
  (let [data (take size (cycle ["abc" 5 :k]))]
    (dotimes [_ reps]
      (time (doall (map mm data))))))

And results:

;; Without patch:
user=> (perf 5 100000)
"Elapsed time: 1301.262 msecs"
"Elapsed time: 928.888 msecs"
"Elapsed time: 942.905 msecs"
"Elapsed time: 858.513 msecs"
"Elapsed time: 832.314 msecs"

;; With patch:
user=> (perf 5 100000)
"Elapsed time: 134.169 msecs"
"Elapsed time: 28.859 msecs"
"Elapsed time: 45.452 msecs"
"Elapsed time: 13.189 msecs"
"Elapsed time: 13.42 msecs"

Attached patch caches the mapping of unknown value -> default dispatch method and significantly improves the performance for this case.

Patch: clj-1429.patch
Screened by: Stu






[CLJ-1427] restart-agent is ignored inside an fn passed to set-agent-handler. It shall maybe documented ? Created: 19/May/14  Updated: 20/May/14  Resolved: 20/May/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Rafik NACCACHE Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Linux, jdk 1.7, emacs / cider



 Description   

If I pass a function containing start-agent to set-error-handler of an agent, if an exception occurs the restart-agent is ignored.
for example:

(def a (agent 0))

(set-error-handler! a (fn [the-agent the-exception] (restart-agent the-agent)) )

If I now issue : (send! a #(/ 1 0)), I still have a failed agent. It did not restart.

I know I can set the error-mode to the agent to :continue to someone have my agent up after a crash, but I wished I could fix the conditions that caused the exception and restart the agent programmatically in the set-error-handler.

Maybe it is a known feature, but it is not documented.



 Comments   
Comment by Rafik NACCACHE [ 20/May/14 11:15 AM ]

Sorry, I probably hit return while typing, this one is duplicate of clj-1428





[CLJ-1426] "/" in keyword leads to unexpected behavior when calling `name` Created: 18/May/14  Updated: 21/May/14  Resolved: 21/May/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Darrell Hamilton Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Because `clojure.lang.Keyword` delegates it's `getName` functionality to `clojure.lang.Symbol`, there is some unexpected behavior when calling `name` on a keyword that contains a "/" in it. For example:

(name (keyword "foo/bar"))
=> "bar"

This is due to `Symbol` stripping all namespace qualifiers and considering only the content trailing the "/" as part of the name.



 Comments   
Comment by Darrell Hamilton [ 20/May/14 9:29 PM ]

Totally misunderstood the behavior of namespaced keywords. this can be closed.

/derp





[CLJ-1421] It's not a digit but regexp classify this one as a digit. Created: 14/May/14  Updated: 14/May/14  Resolved: 14/May/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ale Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None

Attachments: PNG File scr.png    

 Comments   
Comment by Ale [ 14/May/14 12:53 AM ]

[org.clojure/clojure "1.5.1"]

Comment by Ale [ 14/May/14 4:20 AM ]

I have no possibility to close this issue. It was wrong "\" before "1".





[CLJ-1417] clojure.java.io/input-stream has incorrect docstring Created: 07/May/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Trivial
Reporter: Dario Bertini Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft, io

Attachments: File clj-1417.diff    
Patch: Code
Approval: Ok

 Description   

clojure/java/io.clj line 125

"Default implementations are defined for OutputStream, File, URI, URL,"

Should read

"Default implementations are defined for InputStream, File, URI, URL,"

Screened by: Alex Miller






[CLJ-1415] Keyword cache cleanup incurs linear scan of cache Created: 06/May/14  Updated: 07/Oct/14  Resolved: 01/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Alex Miller
Resolution: Not Reproducible Votes: 6
Labels: keywords, performance

Attachments: File faster-keywords.diff     File keyword-cache.diff     Text File kw-clean-future.patch     File unified-kw-patch.diff    
Patch: Code
Approval: Vetted

 Description   

If the GC reclaims a keyword, any subsequent attempt to create a keyword requires an O(n) scan over the entire keyword table via Util.clearCache. This is a significant performance cost in keyword-heavy operations; e.g. JSON parsing.

Patch: keyword-cache.diff - patch to defer cleaning till portion of the table is dead and avoid multiple threads cleaning simultaneously.

Patch: kw-clean-future.patch - patch to spin cache cleaning into a future. Found that 1) this introduces some ordering constraints and circularity between Agent and Keyword (fixable) and 2) using the future pool for this means shutdown-agents would always need to be called (in the patch I avoided this by changing agent's soloExecutor to use daemon threads.

Patch: unified-kw-patch.diff - Alternative to keyword-cache and clean-future.patch. Combines all keyword-perf changes, including the EDN kw parser improvement, improved table lookup performance, and has threads cooperate to empty the table refqueue with a minimum of contention.



 Comments   
Comment by Alex Miller [ 06/May/14 5:53 PM ]

Any perf-related ticket will need some clear before/after timings (with good methodology and how to repro) and also a consideration of cases where the change may introduce any perf degradation in normal usage.

Comment by Kyle Kingsbury [ 07/May/14 9:54 PM ]

I've experimented with a patch reducing the cache clearing cost and removing the need for String.intern. Preliminary results are good, but I want to try a few alternative approaches for cache keys. For instance, could we use pure strings like "foo" and "clojure.core/foo" as the cache keys, removing a level of memory indirection? If we're being really sneaky, we could share those same strings with the Symbol _str field to halve our memory use, assuming it's OK to reach in and mutate it.

https://gist.github.com/aphyr/f72e72992dade4578232
http://imgur.com/a/YSgUa#2

Comment by Alex Miller [ 08/May/14 12:29 PM ]

Great start on this - having the perf data is hugely important. One thing I don't see you've covered yet is what the corresponding memory increase you're incurring with CacheKey to get the benefit - we need to quantify both sides of the tradeoff here (latency/throughput vs memory) to fully judge.

Questions/comments on your patch...

1) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L101 - do we need the (o instanceof CacheKey) check? If the usage of this is constrained then we might be able to skip it (and it will blow up on the next line if something is wrong).

2) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L110 - shouldn't we precompute and save the hash code!? The only thing we're making this for is fast hash comparisons. That hash computation is string length dependent - it ain't cheap.

3) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification for this.

4) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification that this is a reasonable number.

5) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L169 - there is a race here (actually more than one if you include getting the tableSize):

Th1: orphansCount = orphans.get()
Th2: orphansCount = orphans.get()
Th2: orphansNew = orphans.getAndSet(0)
Th2: orphansNew > orphansCount -> start cleaning
<huge gc, 10 zillion orphans are created>
Th1: orphansNew = orphans.getAndSet(0)
Th1: orphansNew > orphansCount -> start cleaning

but I guess this is "safe"; we just have multiple threads cleaning in that case.

6) In general it seems pretty sloppy (I don't mean that pejoratively) how the orphans, rq, and cleaning thread are working together and may be out of sync. I get it and I even believe it will work (usually) but I worry about timing issues that I am too dumb to think of.

Is there a simpler strategy? What if every Nth call to intern() cleaned M entries (or up to M% of table)?

7) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L177 - if you made the iterator explicit in this loop, it would be safe to call iterator.remove() instead of table.remove() and I believe that would be faster on CHM.

8) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L198 - could have two versions of this with/without the symbol. Not sure if that would be faster but they might both inline better into their callers then?

9) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L242 - what's the use case for finding an external CacheKey? Fast re-lookup for specialized use? Do we want to commit to this in the API?

Comment by Kyle Kingsbury [ 08/May/14 2:41 PM ]

1) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L101 - do we need the (o instanceof CacheKey) check? If the usage of this is constrained then we might be able to skip it (and it will blow up on the next line if something is wrong).

I'm usually wary of violating equality/hashCode contracts, and this doesn't even appear as a blip on the radar in profiling. I think we could drop it

2) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L110 - shouldn't we precompute and save the hash code!? The only thing we're making this for is fast hash comparisons. That hash computation is string length dependent - it ain't cheap.

It's less memoizable than you might think; each CacheKey is only indexed a few times, and only at query time; it also doesn't help us for equality checks, since those only occur after hashing. I can add a memoizing field for it at the cost of another 32 bits/kw; we'll see how it impacts performance.

3) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification for this.

I experimented with several values on the Clojure test suite, benchmarks, and some real-world hadoop code. Diminishing returns, as you'd expect. 0.1 and 0.5 are essentially identical in runtime tradeoff. We could drop to 0.01 if desired; it'll only make a difference in large (10-100K) extant keyword benchmarks, as far as I can tell.

4) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification that this is a reasonable number.

Same question as #3?

5) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L169 - there is a race here (actually more than one if you include getting the tableSize):

Th1: orphansCount = orphans.get()
Th2: orphansCount = orphans.get()
Th2: orphansNew = orphans.getAndSet(0)
Th2: orphansNew > orphansCount -> start cleaning
<huge gc, 10 zillion orphans are created>
Th1: orphansNew = orphans.getAndSet(0)
Th1: orphansNew > orphansCount -> start cleaning

but I guess this is "safe"; we just have multiple threads cleaning in that case.

Yep. This check is only there as an optimization--and note that if a huge GC occurs, it's likely we want to schedule a followup traversal of the table anyway, because the thread that's already cleaned some of the table has probably missed some subsequently GC'ed elements. The number of concurrently cleaning threads is bounded by the rate of GC churn, and in the most pathological case (sadly, I haven't been able to produce this race experimentally), this degenerates to the old Clojure behavior of every thread doing a full scan.

6) In general it seems pretty sloppy (I don't mean that pejoratively) how the orphans, rq, and cleaning thread are working together and may be out of sync. I get it and I even believe it will work (usually) but I worry about timing issues that I am too dumb to think of.

Is there a simpler strategy? What if every Nth call to intern() cleaned M entries (or up to M% of table)?

Every nth call is just fine, but it degrades more poorly for large tables. In general, I try to lean towards scale-invariant solutions, which is why I aimed to reclaim roughly a tenth of the entries in the map every time. Maybe more, maybe less, depending on CAS retries, delayed threads resetting the counter to zero, etc.

Doing M entries or M% is more tricky; gotta figure out which threads collect what fraction when, how you efficiently access that subsection of the hash, make sure elements don't fall through the cracks, etc.

7) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L177 - if you made the iterator explicit in this loop, it would be safe to call iterator.remove() instead of table.remove() and I believe that would be faster on CHM.

I agree. I figured Rich had a good reason for doing it this way, but if you concur I'll change it.

8) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L198 - could have two versions of this with/without the symbol. Not sure if that would be faster but they might both inline better into their callers then?

I agree. We can do that dispatch statically and cut down on branch misprediction, too.

9) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L242 - what's the use case for finding an external CacheKey? Fast re-lookup for specialized use? Do we want to commit to this in the API?

Keep forgetting Java's obsession with encapsulation. I'll privatize.

Comment by Alex Miller [ 08/May/14 10:14 PM ]

On several of these - 2, 7, 8 - I think those are worth a test. If faster, we should consider.

On 9, I thought maybe you were opening it up so it would be possible to save off a CacheKey and reuse it or something else. If it's not needed externally, then might be good to private-ize CacheKey itself so we can change it later.

Comment by Kyle Kingsbury [ 09/May/14 6:04 PM ]

http://imgur.com/a/1bv3P#0
https://gist.github.com/aphyr/f72e72992dade4578232

These charts show the performance impact of several changes. In order, they are:

1.7                 baseline
kw                  initial patch
kw-static-paths     Separate codepaths for interning symbols vs strings. Iterator
                    .remove for cache cleaning. Fix a bug for null comparisons
                    in CacheKey namespaces. Internal functions now protected, not
                    public. Not much performance impact.
kw-memo-hash        Memoize hashcodes for CacheKeys. Performance is a wash.
kw-string-cachekeys Observing that String.indexOf('/') consumed a significant 
                    fraction of interning time, use a combined "ns/name" string for
                    Cachekeys instead of separate strings. Significant performance 
                    boost in all tests; 40% reduction in median latencies in 1000-
                    kw allocation test, for instance.
kw-string-keys      Use raw strings for CacheKeys. Improves performance by removing
                    a level of memory indirection, even without cached hashcodes.
kw-interned-keys    Intern those strings to reduce memory consumption, sharing
                    them with the underlying symbol's strings. Slightly slower.

Performance is even better now. Creating 1000 keywords median latency changed from 900 to 200 micros; .999s lower, throughput from 4000 to 20,000/second. JSON parsing median latency fell from 170 micros to 100 micros; throughput doubled from 17500 docs/sec to 36,000 docs/sec.

We're still suffering from poor dispersal in ConcurrentHashmap's use of the string hashCode on JDK7/8, but maybe that's a subject for a different patch.

Memory impact is now minimal. We intern every key string in the table, and those strings are interned by the symbols anyway, so they're essentially the same object. For namespaced symbols, we pay a slightly higher cost--forcing the interning of the "ns/name" string instead of deferring it to Symbol.toString() time. For non-namespaced symbols, these strings are interned as a part of the symbol creation process so there's no memory overhead.

At the repl, I tested by allocating and retaining a million keywords:

(def x (mapv keyword (map (partial str "test-kw-") (range 1e6))))

Retained size (bytes)             1.7   string-kw
----------------------------------------------------
Total retained heap        221.    MB  221.    MB
clojure.lang.Symbols       104.820 MB   32.900 MB
clojure.lang.Keywords       24.021 MB   56.049 MB
java.lang.Strings           89.537 MB   81.786 MB
clojure.lang.Keyword class  72.447 MB   72.451 MB

Total memory use is unchanged, but note that clojure.lang.Symbol retains less, since its strings are now shared by the Keyword table. Keywords, by contrast, retains more. Strings and the keyword table are essentially unchanged.

Comment by Kyle Kingsbury [ 09/May/14 6:08 PM ]

I can't figure out how to edit the ticket description, but I updated the same gist with the cumulative changes. Comments welcome!

Comment by Alex Miller [ 09/May/14 9:51 PM ]

Excellent, thanks for the data. I added a group to your auth so I think you should be able to edit descriptions now. If not, let me know. I'll re-review the patch next week. It would be good either at this point or after that to turn this into an actual patch file instead of a gist.

Comment by Kyle Kingsbury [ 12/May/14 4:24 PM ]

I've attached a cumulative patch. It's comprised of 8 commits; one for each stage we've discussed. I can rebase into a single commit if you'd like.

Comment by Alex Miller [ 13/May/14 7:31 AM ]

I would like a single cumulative rebased patch. I hope to have some time to look at it today.

Comment by Alex Miller [ 13/May/14 12:39 PM ]

On another look, I think it would be useful to separate this ticket into two parts - the first is about optimizing keyword creation and lookup to avoid unnecessary work (avoiding symbol creation and interning, using Strings as keys in the cache). The second part is really about optimizing cache clearing. Do you think these can be separated into two tickets?

Regarding the cache clearing part, have you tested a simpler strategy of just counting calls to clearCache() and running the clean scan every N calls? If that was almost as good, I'd be in favor of that over what is in the patch.

The kw-static paths version did not seem to be an improvement - perhaps you should try pulling them back together to simplify the code? Only worth it if there is a real improvement from it.

On the various find methods - if you could retain their ordering and minimize noise in the diffs that would really help make it easier to screen.

Finally, we need to do some tests to verify that we have not altered the performance of using keywords and symbols as keys in a map for lookup.

Comment by Kyle Kingsbury [ 05/Jun/14 2:36 PM ]

> On another look, I think it would be useful to separate this ticket into two parts - the first is about optimizing keyword creation and lookup to avoid unnecessary work (avoiding symbol creation and interning, using Strings as keys in the cache). The second part is really about optimizing cache clearing. Do you think these can be separated into two tickets?

Created dev.clojure.org/jira/browse/CLJ-1439 for reduced intern cost

> Regarding the cache clearing part, have you tested a simpler strategy of just counting calls to clearCache() and running the clean scan every N calls? If that was almost as good, I'd be in favor of that over what is in the patch.

I'm not confident that this work will be merged, so I'm kinda reticent to go off and spend another N hours doing benchmarks.

> The kw-static paths version did not seem to be an improvement - perhaps you should try pulling them back together to simplify the code? Only worth it if there is a real improvement from it.

It was obsoleted by a later commit; only included it in the benchmark because you asked about the perf impact.

> On the various find methods - if you could retain their ordering and minimize noise in the diffs that would really help make it easier to screen.

Done.

> Finally, we need to do some tests to verify that we have not altered the performance of using keywords and symbols as keys in a map for lookup.

This doesn't touch the lookup path; costs are equivalent.

Comment by Alex Miller [ 09/Jun/14 1:53 PM ]

reduced patch with only the keyword cache clearing changes

Comment by Alex Miller [ 09/Jun/14 8:53 PM ]

Patch that spins cache cleaning into a future

Comment by Kyle Kingsbury [ 21/Jul/14 2:20 PM ]

Just as a followup: got bit by this issue again in a data analysis project today: JSON parsing thrashes the reference queue really hard. Merging this patch would definitely be appreciated. Yourkit screenshot here: http://aphyr.com/media/clojure-keyword-refqueue.png

Comment by Kyle Kingsbury [ 21/Jul/14 4:58 PM ]

Oh yeah, once these two are merged, here's a patch that speeds up my EDN parsing-heavy hadoop jobs by 2-3x. Avoids constructing the symbol every time.

--- a/src/jvm/clojure/lang/EdnReader.java
+++ b/src/jvm/clojure/lang/EdnReader.java
@@ -299,10 +299,9 @@ private static Object matchSymbol(String s){
                        return null;
                        }
                boolean isKeyword = s.charAt(0) == ':';
-               Symbol sym = Symbol.intern(s.substring(isKeyword ? 1 : 0));
                if(isKeyword)
-                       return Keyword.intern(sym);
-               return sym;
+                       return Keyword.intern(s.substring(1));
+               return Symbol.intern(s);
                }
        return null;
 }
Comment by Kyle Kingsbury [ 21/Jul/14 6:33 PM ]
public static void clearCache() {
  if(rq.poll() != null) {
    Agent.soloExecutor.submit(new Runnable() {
      public void run() {
        Util.clearCache(rq,table);
      }
    });
  }
}

This spawns literally hundreds of new threads – 30-40 at a time in my workload – which fight over the referencequeue. Also it causes a fair bit of contention on the executor itself during keyword-heavy allocation-all threads have to synchronize on the executor's queue-but that seems secondary to the cost of the cache-clearing threads themselves.

How about adding a single-thread executor to Agent for these sorts of housekeeping jobs?

Comment by Alex Miller [ 21/Jul/14 8:14 PM ]

I actually built another patch that did exactly that but never got around to attaching it; a single-threaded executor reserved solely for cache clearing. I tried actually making it completely independent but I found it was pretty easy for it to fall behind - it needs to be connected into the construction process to avoid blowing the queue up too big.

I have not been able to build an isolated test that actually showed any significant performance difference with just your cache-clearing change (what's currently attached as keyword-cache.diff) and not the other changes. I had many problems getting your test code to run but when I did get something to run I was not able to see any significant difference with just the keyword-cache.diff.

Comment by Kyle Kingsbury [ 22/Jul/14 6:43 PM ]

Managed to eliminate the refqueue contention by having only one thread involved in GCing at a time. Also doesn't require messing with background threads, and is less susceptible to the queue-overflow problem. Since the various extant patches don't apply cleanly on top of each other, I've re-written them in unified-kw-patch.diff, attached. Roughly doubles throughput compared to your patch, at least on a 24-core xeon running openjdk7.

http://aphyr.com/media/clojure-reduced-kw-refqueue-contention.png

Can you please reconsider merging? I've put over a hundred hours into writing, testing, and refining this patchset; it's been stable in production for months and has made a dramatic difference in several of our data-heavy Clojure programs.

Comment by Alex Miller [ 23/Jul/14 10:58 AM ]

Hey Kyle, I appreciate the time you've put into this. However, having a big giant patch tuned on a single use case is not an effective way to evolve the language. We need to separate and describe problems, then explore the solution space for each one, as independently as possible, while considering the impacts on all other use cases.

This particular ticket is concerned solely with the linear cleanup of the reference queue. Can you split out just a patch that deals with this issue? It would be helpful to have a test that demonstrates the performance problem and how this patch address the problem. My testing so far with the prior patch did not demonstrate any improvement.

It would also be helpful to have a squashed version of the complement of the changes related to interning on CLJ-1439 for consideration of that as a separate problem. (And maybe there is further splitting that could be done; I have not looked closely at the interning changes.)

Comment by Alex Miller [ 23/Jul/14 11:00 AM ]

The EdnReader changes, for example, should be a separate ticket.

Comment by Kyle Kingsbury [ 23/Jul/14 12:30 PM ]

Could you at least merge dev.clojure.org/jira/browse/CLJ-1439 first? I split it into a separate ticket over a month ago and these changes depend on it.

Comment by Alex Miller [ 23/Jul/14 1:27 PM ]

I would be happy to consider CLJ-1439 first. Can you update the patch there to be current and focused on the intern/cache?

Comment by Kyle Kingsbury [ 23/Jul/14 2:35 PM ]

The patch is current, and it is focused on the intern/cache.

Comment by Andy Fingerhut [ 01/Aug/14 9:31 PM ]

Kyle, CLJ-1439 was completed today via a commit to Clojure master. That also had the side effect of making all of the currently attached patches no longer apply cleanly. I haven't checked how easy or difficult it might be to update them to apply cleanly.

Comment by Alex Miller [ 01/Aug/14 11:35 PM ]

I am not able to reproduce a performance issue due to a linear scan of the reference queue cache. Obviously the scan occurs but in most cases the scan is comparatively fast and does not seem to be a bottleneck in the tests I've run.

If there is a test that can reproduce this issue (on current master) and demonstrates an improvement with this patch, please reopen the ticket for investigation.





[CLJ-1408] Add transient keyword to cached toString() value in _str Created: 19/Apr/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Minor
Reporter: Tomasz Nurkiewicz Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408-3.patch     Text File CLJ-1408.patch    
Patch: Code and Test
Approval: Ok

 Description   

_str field in Keyword and Symbol classes lazily caches result of toString(). Because this field is not transient, serializing (using Java serialization) any keyword or symbol before and after calling toString() for the first time yields different results:

(import (java.io ByteArrayOutputStream ObjectOutputStream
                 ByteArrayInputStream  ObjectInputStream))

(defn- serialize [obj]
  (with-open [bos (ByteArrayOutputStream.)
              stream (ObjectOutputStream. bos)]
    (.writeObject stream obj)
    (-> bos .toByteArray seq)))

;; keyword example

(def k1 (serialize :k))
(println :k)
(def k2 (serialize :k))

(= k1 k2) ;;=> false 

;; symbol example

(def sym 'a)

(def s1 (serialize sym))
(println sym)
(def s2 (serialize sym))

(= s1 s2) ;;=> false

This issue came up when I was trying to use keywords as key in [Hazelcast](https://github.com/hazelcast/hazelcast) map. Hazelcast uses serialized keys in various scenarios, thus if I first put something to map under key :k and then print :k, I can no longer find such key.

Approach: Add transient keyword to _str field in Keyword and Symbol classes

Patch: CLJ-1408-3.patch

Screened by: Brenton Ashworth



 Comments   
Comment by Alex Miller [ 19/Apr/14 7:28 AM ]

Hi Tomasz, it would be good to fix this, can you sign the CLA?

Comment by Tomasz Nurkiewicz [ 20/Apr/14 7:26 AM ]

Thanks, I'll sign and send CLA ASAP.

Comment by Tomasz Nurkiewicz [ 08/May/14 4:10 PM ]

My contributor greement arrived, please merge this patch whenever you find suitable.

Comment by Alex Miller [ 08/May/14 10:16 PM ]

Hi Tomasz, I noticed you added the private keyword - please remove that and update the patch.

Comment by Tomasz Nurkiewicz [ 09/May/14 3:55 PM ]

Removed `private` keyword

Comment by Alex Miller [ 20/Jun/14 9:22 AM ]

On second thought, it looks like we have most of the infrastructure for serialization testing anyways, so would appreciate an updated patch with the example turned into a serialization test. Please see test/clojure/test_clojure/serialization.clj for a place to put this (using existing roundtrip function).

Comment by Andy Fingerhut [ 01/Aug/14 9:29 PM ]

Tomasz, in addition to Alex's previous comment, it appears that a commit made to Clojure master earlier today causes your patches to no longer apply cleanly. I haven't looked to see whether updating the patches would be easy, but likely it is.

Comment by Alex Miller [ 22/Aug/14 11:00 PM ]

Updated the patch for latest master and added the obvious test.





[CLJ-1405] clojure runtime does not work with onejar-maven-plugin Created: 16/Apr/14  Updated: 06/Jul/14  Resolved: 06/Jul/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6, Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Sean Shubin Assignee: Unassigned
Resolution: Duplicate Votes: 1
Labels: None
Environment:

I was able to repeat this problem on windows, linux, and osx


Attachments: Text File onejar-maven-plugin-fix.patch    
Patch: Code

 Description   

I have created a sample project, steps to repeat the problem, and a proposed patch here:
https://github.com/SeanShubin/clojure-one-jar

I have also attached my proposed patch to this ticket, and pasted the relevant portion of README.md below:

clojure-one-jar
===============

Sample project to demonstrate problem with combining clojure runtime with onejar-maven-plugin

Steps to repeat the problem
===========================

  • mvn package
  • java -jar target/greeter.jar

Behavior before patch is applied
================================
Exception in thread "main" java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at com.simontuffs.onejar.Boot.run(Boot.java:340)
at com.simontuffs.onejar.Boot.main(Boot.java:166)
Caused by: java.lang.ExceptionInInitializerError
at com.seanshubin.clojure_one_jar.Greeter.main(Greeter.java:12)
... 6 more
Caused by: java.lang.NullPointerException
at clojure.lang.RT.lastModified(RT.java:387)
at clojure.lang.RT.load(RT.java:421)
at clojure.lang.RT.load(RT.java:411)
at clojure.lang.RT.doInit(RT.java:447)
at clojure.lang.RT.<clinit>(RT.java:329)
... 7 more

Behavior after patch is applied
===============================
No need to call RT.init() anymore
Hello, world!

Patch
=====
I applied this patch to my local copy of clojure 1.7.0-master-SNAPSHOT
https://github.com/SeanShubin/clojure-one-jar/blob/master/onejar-maven-plugin-fix.patch



 Comments   
Comment by Alex Miller [ 16/Apr/14 9:45 PM ]

I think this is a dupe of CLJ-971 - can you verify?

Comment by Sean Shubin [ 16/Apr/14 9:49 PM ]

Yes, I looked over CLJ-971 and can confirm this matches my observations regarding why this is happening.

Comment by Sean Shubin [ 16/Apr/14 9:54 PM ]

My solution is a bit different in that it still tries to get the date of the file rather than defaulting to the date of the entire jar. I don't have a solid enough understanding of the calling code to know if this is important.

Comment by Alex Miller [ 16/Apr/14 10:17 PM ]

FYI, to consider your patch, I would need you to sign a Contributor Agreement - see http://clojure.org/contributing for details. I do think that is an area that something is needed, will need to assess the options a bit more.

Comment by Sean Shubin [ 16/Apr/14 10:56 PM ]

Sure thing, I just signed and sent a Contributor Agreement, per the instructions at http://clojure.org/contributing

Comment by Gus Heck [ 05/Jul/14 9:42 PM ]

This effects the one-jar-gradle plugin too. Any progress on reviewing Sean's patch?

Comment by Sean Shubin [ 06/Jul/14 12:13 AM ]

Gus, I suggest moving this conversation over to CLJ-971.

The main problem is that I had not time to get it under test coverage, see Stuart Halloway's comment regarding that on CLJ-971. I have been pretty busy with my other projects, and being that I am not familiar with the clojure code base, I am going to have to block out some time to figure out how to get the appropriate tests in place, perhaps I will have some time for that next weekend.

Comment by Alex Miller [ 06/Jul/14 6:11 PM ]

Same issue as CLJ-971, which is farther along in the process.





[CLJ-1404] clojure.core/vals returns nil on an empty map instead of an empty sequence Created: 14/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Satshabad Khalsa Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Is this a bug? maybe I just don't understand. The documentation says: Returns a sequence of the map's values. Is nil a sequence?

This caused an unexpected nil to propagate through a bunch of list processing stuff.



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:54 PM ]

An empty sequence is represented by nil, so this is consistent. For example: (seq (range 0)) => nil





[CLJ-1397] exception testing broken over map Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: test
Environment:

Linux ... 3.2.0-56-generic-pae #86-Ubuntu SMP ... i686 i686 i386 GNU/Linux



 Description   

Expected: Tests pass
Actual: Two tests fail
To reproduce, run the following test file:

(ns pe.test-test
(:require [clojure.test :refer :all]))
(defn throwexc [m] (throw (Exception. m)))
(defn throwass [m] (assert false m))
(defn nestexc [] (throwexc "exc"))
(defn nestass [] (throwass "ass"))
(defn nestmapexc [] (map throwexc '("a" "b" "c")))
(defn nestmapass [] (map throwass '("a" "b" "c")))
(deftest exceptions-and-assertions-test
(testing "throwing"
(is (thrown? Exception (throwexc "exc")))
(is (thrown? AssertionError (throwass "ass"))))
(testing "nesting"
(is (thrown? Exception (nestexc)))
(is (thrown? AssertionError (nestass))))
(testing "nesting over map"
(is (thrown? Exception (nestmapexc)))
(is (thrown? AssertionError (nestmapass)))))



 Comments   
Comment by MG [ 01/Apr/14 7:25 AM ]

Clarification: The two assertions in "nesting over map" fails, the other four assertions succeed.

Comment by Nicola Mometto [ 01/Apr/14 7:30 AM ]

map is lazy, the exception is never thrown because the sequence is never realized.
either wrap the map in a dorun or use a doseq instead of a map.
map should not be used for side-effecting





[CLJ-1396] Bad link Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

http://richhickey.github.io/clojure/



 Description   

http://richhickey.github.io/clojure/
The issue tracker link points to Assembla.



 Comments   
Comment by Nicola Mometto [ 01/Apr/14 7:31 AM ]

That site is deprecated and so is the repo that's hosting it.
This is the updated one http://clojure.github.io/clojure/

Comment by Alex Miller [ 01/Apr/14 7:45 AM ]

Nicola, I agree with your assessment here but only screeners should be closing tickets, thanks.

Comment by Nicola Mometto [ 01/Apr/14 7:50 AM ]

Alex, sorry, I didn't know this, I will refrain from doing so in the future.





[CLJ-1395] get should deref delay, refs atoms etc Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(get (delay {:a 1}) :a)
;; nil

(get {:a 1} :a)
;; 1

The above situation can happen easily and there is no way to refactor or check that all code doing gets is in fact doing deref before doing get. Given that everything is dynamic typing, changing a single value from a map to delay or ref can make large parts of the code fail silently on nil for gets.

get would be more consistent (with what is expected of it) if it was to check for refs, atoms and delays and do a deref.



 Comments   
Comment by Alex Miller [ 01/Apr/14 7:43 AM ]

I think this goes beyond what get should do. In regards to silent failure, that is covered by CLJ-1107 which would cause this to throw an exception instead.





[CLJ-1393] clojure.string/trim doesn't trim null character Created: 28/Mar/14  Updated: 28/Mar/14  Resolved: 28/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

CLJ-935 changed clojure.string/trim to not trim all the characters less than or equal to \u0020 as Java does.

I noticed this because base64 uses null characters to pad the end of encoding blocks.

Clojure 1.6.0's trim leaves the null character in:
user=> (.length (clojure.string/trim "\u0000"))
1

java.lang.String's trim takes it out:
user=> (.length (.trim "\u0000"))
0

Here are the first 21 unicode characters and what Character/isWhitespace says about them.

(dotimes [n 0x20] (printf "
u%04x - %b\n" n (Character/isWhitespace n)))
\u0000 - false
\u0001 - false
\u0002 - false
\u0003 - false
\u0004 - false
\u0005 - false
\u0006 - false
\u0007 - false
\u0008 - false
\u0009 - true
\u000a - true
\u000b - true
\u000c - true
\u000d - true
\u000e - false
\u000f - false
\u0010 - false
\u0011 - false
\u0012 - false
\u0013 - false
\u0014 - false
\u0015 - false
\u0016 - false
\u0017 - false
\u0018 - false
\u0019 - false
\u001a - false
\u001b - false
\u001c - true
\u001d - true
\u001e - true
\u001f - true



 Comments   
Comment by Alex Miller [ 28/Mar/14 12:27 PM ]

The choice was made in CLJ-935 to consistently define whitespace as Character.isWhitespace() across trim, triml, and trimr. There are many possible ways to define "space" (at least two as we see here). If your trimming needs differ from the standard library, then you'll probably need to define your own functions to trim your data. You can still use Java interop to call String.trim() directly if that happens to match your needs.

Comment by Ryan Fowler [ 28/Mar/14 1:03 PM ]

Indeed, it's an easy workaround to use Java interop once you figure out what your problem is.

It's just unintuitive that the character generally used for string termination isn't trimmed by clojure.string/trim.





[CLJ-1392] AOT vs "var already exists warning" results in NPE Created: 26/Mar/14  Updated: 26/Mar/14  Resolved: 26/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: aot, compiler


 Description   

I just saw this thread on the cascalog list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Apparently the "WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?" results in a NullPointerException when trying to AOT a namespace that results in the message being output.

Reproducer:

1) lein new aotFail
2) Edit project.clj
;add as appropriate
:aot :all
:dependencies [[org.clojure/clojure "1.6.0"]
[cascalog "2.0.0"]]
2) Add "(:use cascalog.api)" to the ns block of src/aotFail/core.clj
3) lein compile

Output:

Compiling aotFail.core
WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?
Exception in thread "main" java.lang.NullPointerException, compiling:(api.clj:1:1)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3558)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5528)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog2.core$loading_4958auto_.invoke(core.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$compile$fn__5071.invoke(core.clj:5652)
at clojure.core$compile.invoke(core.clj:5651)
at user$eval19.invoke(form-init2092370125048380878.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6703)
at clojure.lang.Compiler.eval(Compiler.java:6693)
at clojure.lang.Compiler.load(Compiler.java:7130)
at clojure.lang.Compiler.loadFile(Compiler.java:7086)
at clojure.main$load_script.invoke(main.clj:274)
at clojure.main$init_opt.invoke(main.clj:279)
at clojure.main$initialize.invoke(main.clj:307)
at clojure.main$null_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:420)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4944)
at clojure.lang.Compiler$DefExpr.emit(Compiler.java:437)
at clojure.lang.Compiler.compile1(Compiler.java:7225)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5524)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog.api$loading_4958auto_.invoke(api.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)



 Comments   
Comment by Nicola Mometto [ 26/Mar/14 12:42 PM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1241





[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File 0001-FIX-CLJ-1388.patch     Text File CLJ-1388.patch     Text File CLJ-1388-record-equality-and-map-record-factory.patch     Text File CLJ-1388v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

Cause: The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: CLJ-1388-record-equality-and-map-record-factory.patch

Screened by: Alex Miller



 Comments   
Comment by Steve Miner [ 28/Apr/14 3:06 PM ]

The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]

Comment by Steve Miner [ 28/Apr/14 3:10 PM ]

It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.

Comment by Alex Miller [ 28/Apr/14 3:23 PM ]

Moving out of Screened due to new patch

Comment by Nicola Mometto [ 28/Apr/14 3:35 PM ]

Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type.

Here's an example that returns true with my patch, but still returns false with yours.

user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Comment by Nicola Mometto [ 28/Apr/14 3:37 PM ]

It should also be noted that the overhead of copying the record map is probably insignificant.

Comment by Nicola Mometto [ 28/Apr/14 3:42 PM ]

I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:

  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details

I think patching the map->record function is the best way to go.

Comment by Steve Miner [ 28/Apr/14 3:56 PM ]

Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.

Comment by Steve Miner [ 29/Apr/14 10:17 AM ]

Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.

Comment by Steve Miner [ 29/Apr/14 10:46 AM ]

A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's.

The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map.

The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence.

There might be a better way to fix the Record/create method but I couldn't figure it out.

Comment by Nicola Mometto [ 29/Apr/14 1:52 PM ]

Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=`

This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way.

There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map.

Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.

Comment by Steve Miner [ 29/Apr/14 2:10 PM ]

Whatever works for you regarding the tests is fine by me.

Comment by Alex Miller [ 30/Apr/14 12:07 AM ]

It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later.

In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean?

Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?

Comment by Nicola Mometto [ 30/Apr/14 4:15 AM ]

Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way.

Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap)

Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.

Comment by Steve Miner [ 30/Apr/14 8:27 AM ]

My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.

Comment by Nicola Mometto [ 30/Apr/14 3:57 PM ]

CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map.

To summarize, here's the status of the different patches for this ticket:

  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Comment by Alex Miller [ 05/May/14 1:59 PM ]

Are these patches all still in play? Having 4 active patches does not help move a ticket forward.

Can someone re-summarize at this point what questions exist?

Comment by Nicola Mometto [ 06/May/14 5:26 AM ]

0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way.

To pick between the other patches, we need to chose which approach to go with.
Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map.

I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record.

To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch

Comment by Alex Miller [ 06/May/14 10:22 AM ]

From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.

Comment by Alex Miller [ 23/May/14 11:19 AM ]

Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.





[CLJ-1387] reduce-kv on hash map ignores reduced objects in large maps Created: 18/Mar/14  Updated: 22/Mar/14  Resolved: 22/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: None
Environment:

JRE 7


Attachments: File clj-1387.diff     File clj-1387-v2.diff     File clj-1387-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Larger hash maps have nested INodes. As kvreduce implementations in INodes dereference reduced objects, parent INodes continue to reduce.

user=> (defn test-reduce-kv [m] (reduce-kv (fn [_ k v] (when (== 1 k) (reduced :foo))) nil m))
#'user/test-reduce-kv
user=> (test-reduce-kv (zipmap (range 3) (range 3)))
:foo
user=> (test-reduce-kv (zipmap (range 300) (range 300)))
nil

Dereferencing reduced objects should happen only PersistentHashMap/kvreduce - intermediate nodes should pass the Reduced object along.

Patch: clj-1387-v3.diff
Screened-by:



 Comments   
Comment by Alex Miller [ 18/Mar/14 5:11 PM ]

I updated the patch to use a generative test that will try many combinations of map size and the reduced index to bail out on. This test failed before applying the source patch and passes with it.

Comment by Rich Hickey [ 21/Mar/14 7:33 AM ]

if(root != null){ - return root.kvreduce(f,init); + init = root.kvreduce(f,init); + if(RT.isReduced(init)) + return ((IDeref)init).deref(); }

Turns code that always had a return into code that sometimes does.

Comment by Alex Miller [ 21/Mar/14 9:07 AM ]

Added new version of patch that retains the return flow and doesn't fall through.





[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: performance

Attachments: Text File CLJ-1384-p1.patch     Text File CLJ-1384-p2.patch     File set-bench.tar    
Patch: Code
Approval: Ok

 Description   

CLJ-1384-p2 uses transients for both create and createWithCheck. This is consistent with the current implementation for map.

clojure.core/vec calls (more or less) PersistentVector.create(...), which uses a transient vector to build up the result.

clojure.core/set on the other hand, calls PersistentHashSet.create(...), which repeatedly calls .cons on a PersistentHashSet, with all the associated speed/GC issues.

Operation count now w/transients
set 5 1.771924 µs 1.295637 µs
into 5 1.407925 µs 1.402995 µs
set 1000000 2.499264 s 1.196653 s
into 1000000 0.977555 s 1.006951 s

Patch: CLJ-1384-p2.patch
Screened by: Stu



 Comments   
Comment by Gary Fredericks [ 15/Mar/14 10:13 PM ]

PersistentHashSet has six methods for creating sets – one for each combination of {with check, without check} and {array (varargs), List, ISeq}. Each of them does not use transients but could.

I believe clojure.core/set only depends on the (without check, ISeq) version.

Should all of these be changed? Three of them? One of them?

Comment by Andy Fingerhut [ 15/Mar/14 10:21 PM ]

I believe that the 'with check' versions are only intended to be used when reading set literals in Clojure source code, and give an error if there are duplicate elements. If you find examples where those set creation functions are called in other situations, I would be interested to learn about them to find out where my misunderstanding lies, or whether that is a problem with the current code.

If the belief above is correct, I would suggest not changing the 'with check' versions, since their speed isn't as critical.

Comment by Gary Fredericks [ 15/Mar/14 10:23 PM ]

Thanks Andy, I'll submit a patch that changes the three non-checked methods.

Comment by Gary Fredericks [ 15/Mar/14 10:46 PM ]

Attached CLJ-1384-p1.patch, which updates the three non-check create methods.

I also added benchmarks. It's about 2-3 times faster for large collections.

Comment by Ambrose Bonnaire-Sergeant [ 11/Apr/14 11:15 AM ]

Added benchmark suite (set-bench.tar).

FWIW results are similar to gfrederick's on my machine:

Clojure 1.6

Small collections (5 elements)

set averages 1.220601 µs
into averages 1.597991 µs

Large collections (1,000,000 elements)

set averages 2.429066 sec
into averages 1.006249 sec

After transients

Small collections (5 elements)

set averages 999.248325 ns
into averages 1.162889 µs

Large collections (1,000,000 elements)

set averages 1.003792 sec
into averages 889.993185 ms

Add full output to the tar.

Comment by Ghadi Shayban [ 11/Apr/14 11:35 AM ]

CLJ-1192 is related to this, but and Andy seems to be indicating the use of reduce as the means to better performance there.

Comment by Gary Fredericks [ 11/Apr/14 11:41 AM ]

Oh that's a good point about reduce. The difference should only apply to chunked seqs, right? It's worth noting that the benchmarks above used range which creates chunked seqs, so that might be why into looks faster on the large collections?

So this change only makes set act like vec; I think whether either/both of them should use reduce is a different question.





[CLJ-1382] Vector sort order should be specified sufficiently to embrace sort-by-juxt Created: 13/Mar/14  Updated: 15/Mar/14  Resolved: 15/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: data-structures, documentation, idiom


 Description   

Vectors of equal length sort in a way that seems natural – by
comparing their 0th elements, then their 1st, etc., until something
is different:

user> (def vv '(["c" 9] ["a" 100] ["a" 33] ["b" 8]))
#'user/vv
user> (sort vv)
(["a" 33] ["a" 100] ["b" 8] ["c" 9])

This property enables a blisteringly wonderful idiom for sorting
records by multiple keys:

user> (def mm (->> vv (map (fn [[p q]] {:p p :q q}))))
#'user/mm
user> (sort-by (juxt :p :q) mm)
({:p "a", :q 33} {:p "a", :q 100} {:p "b", :q 8} {:p "c", :q 9})

The sort-by-juxt idiom was described on briancarper.net[1], where it
was refined for Clojure 1.1 by Malcolm Sparks. Andy Fingerhut has
also written about it, thoroughly.[2]

Such lore gives it the odor of respectability, but the sort-by-juxt
idiom takes liberties beyond the documented behavior ("contract") of
vectors. APersistentVector indulges the idiom, but the clojure.org
Data Structures page does not say how vectors should compare.

The vector specification should be bolstered with enough traits of
vectors' sorting behavior to make the sort-by-juxt idiom safe to use
wherever Clojure might be implemented.

[1] http://briancarper.net/blog/488/sort-a-clojure-map-by-two-or-more-keys

[2] https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md



 Comments   
Comment by Alex Miller [ 13/Mar/14 9:52 PM ]

I don't understand what this ticket is asking for.

Comment by Andy Fingerhut [ 13/Mar/14 10:32 PM ]

It sounds like he is asking that the doc of clojure.core/compare say that vectors of equal length are compared in lexicographic order.

Comment by Phill Wolf [ 15/Mar/14 1:07 PM ]

"(sort-by (juxt" relies on a feature of vectors that the Clojure documentation does not guarantee. But using juxt in this way is part of the cultural fabric and helps make concise programs that "read like a definition" of the problem, to quote Halloway in "Programming Clojure". Therefore, let's document the sort order of equal-length vectors. I looked for this information on the Data Structures page, which has a section on vectors.

Comment by Alex Miller [ 15/Mar/14 11:11 PM ]

I added a line to the Vectors section on the Data Structures (http://clojure.org/data_structures) page: "Vectors are compared first by length, then each element is compared in order."





[CLJ-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   

Example showing how a local fn can be hinted but an anonymous function cannot:

;; OK
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)
            f (fn [])]
        (.submit ex ^Runnable f))
nil
;; ERROR - this should work the same as the previous
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)]
        (.submit ex #()))
CompilerException java.lang.IllegalArgumentException: More than one matching method found: submit, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init7901279404687292754.clj:3:9)

Cause: Functions have metadata, but Compiler does not look in them for type hints. Var expressions and local bindings use :tag metadata to override return of getJavaClass(). Compiler parses #() into a FnExpr, which always return AFunction as its class.

Proposed: Change FnExpr.getJavaClass() to return tag as type if it is available.

Patch: clj-1378-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





[CLJ-1377] java.lang.IllegalArgumentException: More than one matching method found on calling ExecutorService.submit from let instead of using Var. Created: 11/Mar/14  Updated: 12/Mar/14  Resolved: 11/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Example from Joy Of Clojure doesn't work when Executor service pool is used in (let )

https://groups.google.com/forum/#!msg/clojure/asEXM2uHyxw/aK4rrKOKs1YJ



 Comments   
Comment by Alex Miller [ 11/Mar/14 3:47 PM ]

In the let case, the pool will be tagged with the proper type so the ambiguity is detected.

In the def case, the pool will be seen as an object and the compiler is just deferring to reflection at runtime to figure it out. If you turn on warn-on-reflection, you'll see a reflection warning in this case. Reflection is just picking the first one that matches in that case. If you type hinted the def case, you'd see the same error.

I don't think there is a bug here.

Comment by Roy Varghese [ 11/Mar/14 5:08 PM ]

Actually, I did turn on warn-on-reflection, but didn't see any messages.

I think this creates an element of surprise and uncertainty.

(def A (let [x (java.util.concurrent.Executors/newFixedThreadPool 5)] x))

How would (.submit A ...) behave?

Comment by Alex Miller [ 11/Mar/14 11:15 PM ]

When I ran the prior example, I saw a reflection warning.

In your new example, I would expect x to be typed as a ThreadExecutorPool, A to be typed as an Object, and .submit to get called reflectively (successfully) and produce a reflection warning. It's effectively no different than the def example in the post.

What are you suggesting is the bug and what should happen? Reflector could throw an error when it finds multiple matches but that would certainly break code that is currently working like the example you gave, so I doubt we would consider such a change at this point. There really is an ambiguity here, so I think a type hint is required to make it work unambiguously in either case.

Comment by Roy Varghese [ 12/Mar/14 10:49 AM ]

The bug is that type hints are required in one case, and not required in the other case because of implementation details, unless its documented in the language that (def) and (let) hold different types of Vars.

I the example above, I would expect A and x to refer to the same object, and thus contain the type information. Or not. Without reading the implementation, either case seems possible.

Agree, its a breaking change, but that's a different consideration.

Comment by Roy Varghese [ 12/Mar/14 10:50 AM ]

BTW..trying to fix with hints is what led to CLJ-1378.

Comment by Alex Miller [ 12/Mar/14 11:39 AM ]

x is locally bound to an object (there is no Var here) - the type information is on the local binding, inferred from the type of the expression. In the compiled form, the let expression does know the return type of the evaluated let (if there were multiple branches, there might be many possible return types). The def creates a Var that holds the result of evaluating the let. At compilation time, the type of the result of the let is unknown so is assumed to be Object.

So, x is a local binding and A is a Var. Even if they both refer to the same object, they do so in different contexts using different information. The type hinting and binding behavior in let is described on the special forms page http://clojure.org/special_forms. Remember too that Clojure is a dynamic language - while the Var A might hold a ThreadPoolExecutor instance now, it might hold something else later.

CLJ-1378 is useful - that's phrased more as a problem we can solve and there's a reasonable patch there.

Comment by Alex Miller [ 12/Mar/14 11:55 AM ]

"there is no Var here" == "there is no Var at this point"
"the let expression does know the return type" should have been "does NOT know"





[CLJ-1374] Make PersistentQueue implement List Created: 09/Mar/14  Updated: 31/Mar/14  Resolved: 31/Mar/14

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: File clj-1374-1.diff    
Patch: Code and Test

 Description   

Most ordered Clojure collections like lists, vectors, and lazy seqs implement java.util.List, and thus .equals can be true between values of those types and other collections implementing java.util.List, like java.util.Vector and java.util.ArrayList.

Clojure PersistentQueue seems to be the odd man out here, in that it implements Collection but not List, and thus while it can be .equals to Clojure lists, vectors, and lazy seqs, it cannot be .equals to other collections implementing java.util.List.

user=> (instance? java.util.List '())
true
user=> (instance? java.util.List (lazy-seq))
true
user=> (instance? java.util.List [])
true
user=> (instance? java.util.List (vector-of :long))
true
user=> (instance? java.util.List clojure.lang.PersistentQueue/EMPTY)
false

user=> (= '() (java.util.ArrayList.))
true
user=> (= (lazy-seq) (java.util.ArrayList.))
true
user=> (= [] (java.util.ArrayList.))
true
user=> (= (vector-of :long) (java.util.ArrayList.))
true
user=> (= clojure.lang.PersistentQueue/EMPTY (java.util.ArrayList.))
false


 Comments   
Comment by Andy Fingerhut [ 09/Mar/14 6:29 PM ]

Patch clj-1374-1.diff is written assuming that CLJ-1372 patch clj-1372-2.diff or very similar has been committed, because of the tests modified, not because of the change of PersistentQueue to implement the java.util.List interface. I can update this patch as desired if that change does not go in.

Comment by Andy Fingerhut [ 09/Mar/14 6:33 PM ]

Ugh. The subject is definitely a duplicate of CLJ-1059, which I should have checked for before creating this ticket. I will compare patches to see how the approaches compare. Mine is probably a poor substitute for that one, but the tests I add may still be useful to keep in a patch for CLJ-1059.

Comment by Andy Fingerhut [ 31/Mar/14 5:29 PM ]

Problem description is a duplicate of CLJ-1059. Even the patch (independently developed) is nearly the same as the patch with a name beginning with "001" attached to CLJ-1059.





[CLJ-1370] ((println)) should throw CompilerException instead of NPE? Created: 06/Mar/14  Updated: 06/Mar/14  Resolved: 06/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug


 Description   

How to Reproduce

=> ((println))

NullPointerException   hello.core/eval4117 (NO_SOURCE_FILE:1)

What's Wrong?

I might be completely wrong, in which case don't hesitate to close this defect, but I wish this would throw a CompilerException with an IllegalArgumentException, just like ((nil)):

=> ((nil))
CompilerException java.lang.IllegalArgumentException: Can't call nil, compiling:(NO_SOURCE_PATH:1:2)


 Comments   
Comment by Alex Miller [ 06/Mar/14 12:37 PM ]

There is no compilation error here. The error occurs during evaluation.

user> (defn x [] ((println)))  ;; compiles just fine
#'user/x
user> (x)   ;; fails in evaluation
NullPointerException   user/x (NO_SOURCE_FILE:1)

The error is thrown when trying to evaluate (nil) where NullPointerException is a perfectly valid error.





[CLJ-1369] CLJ-738 is marked Closed is not implemented Created: 04/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: David Welte Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Java 6



 Description   

CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false. See CLJ-738 for many details.



 Comments   
Comment by Alex Miller [ 04/Mar/14 3:20 PM ]

Thanks for letting us know about this - I concur that 738 was incorrectly closed without being applied and I have resurrected that ticket. I am closing this one. In the future, feel free to just comment on a ticket directly, or better (for a closed ticket), comment on one of the mailing lists.





[CLJ-1365] New collection hash functions are too slow Created: 20/Feb/14  Updated: 11/Mar/14  Resolved: 11/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1365-v1.patch     Text File clj-1365-v2.patch     Zip Archive testclj1365.zip    
Patch: Code
Approval: Ok

 Description   

As reported ( https://groups.google.com/d/msg/clojure-dev/t6LAmVe-RLM/ekLTKxYfU5UJ ) by Mark Engelberg, the new collection hashing functions are slower than invoking the Murmur3 functions directly. See the attached zip for performance tests.

Approach: Made mix-collection-hash, hash-ordered-coll, and hash-unordered-coll use primitive type hints to avoid the bulk of the time.

Patch: clj-1365-v2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 20/Feb/14 11:26 AM ]

Added to 1.6 release.

Comment by Alex Miller [ 20/Feb/14 12:40 PM ]

Made hash functions inline for performance.

Comment by Rich Hickey [ 20/Feb/14 7:55 PM ]

Reported where?

This looks like bad benchmarking.

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] (clojure.lang.Murmur3/mixCollHash x y)))))

and

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] #_(clojure.lang.Murmur3/mixCollHash x y)))))

take the same time on my machine.

I'd need to see tests where the return was definitely used, it seems this is just more easily ignored by hotspot when not used.

We probably only need to hint count and the return for decent results.

Comment by Alex Miller [ 20/Feb/14 8:55 PM ]

It was reported by Mark Engelberg in his Instaparse rework - he observed these calls taking noticeably longer and overall times 10-20% down. I will ask him to chime in here.

Comment by Rich Hickey [ 04/Mar/14 8:44 AM ]

Could someone please test hinting hint count and the return? I'd hate for the answer to anyone's perf issues be inlining.

Comment by Alex Miller [ 04/Mar/14 9:06 AM ]

I will provide some more data for consideration of the options.

Comment by Alex Miller [ 04/Mar/14 11:07 AM ]

Test project for different variants

Comment by Alex Miller [ 04/Mar/14 11:11 AM ]

Attached a test project with different variants for testing and better benchmarking. To run:

unzip testclj1365.zip
cd clj1365
lein uberjar
java -server -cp target/clj1365-0.1.0-SNAPSHOT-standalone.jar clj1365.core

Results:

mix-collection-hash original
"Elapsed time: 57.777 msecs"
"Elapsed time: 18.034 msecs"
"Elapsed time: 20.591 msecs"
"Elapsed time: 25.179 msecs"
"Elapsed time: 21.781 msecs"
mix-collection-hash hints
"Elapsed time: 14.983 msecs"
"Elapsed time: 8.871 msecs"
"Elapsed time: 8.793 msecs"
"Elapsed time: 8.92 msecs"
"Elapsed time: 8.873 msecs"
mix-collection-hash inline
"Elapsed time: 10.04 msecs"
"Elapsed time: 7.117 msecs"
"Elapsed time: 7.306 msecs"
"Elapsed time: 7.324 msecs"
"Elapsed time: 7.175 msecs"
Murmur3/mixCollHash
"Elapsed time: 9.522 msecs"
"Elapsed time: 7.288 msecs"
"Elapsed time: 7.397 msecs"
"Elapsed time: 7.364 msecs"
"Elapsed time: 7.345 msecs"

From these results, I infer that the unhinted version is slower (21 ms) than a static call (7 ms). Inlining gives you same perf as static. Hinting inputs and return gives almost the same perf (9 ms).





[CLJ-1363] Field access via .- in reflective case does not work Created: 18/Feb/14  Updated: 28/Feb/14  Resolved: 28/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File clj-1363-v1.patch     Text File clj-1363-v2.patch     Text File clj-1363-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/14 7:24 PM ]

You can't change the semantics of invokeNoArgInstanceMember - they are correct when not using '-'. We need to feed the info that '-' was used through InstanceFieldExpr and make field-first conditional on that.

Comment by Alex Miller [ 21/Feb/14 5:42 AM ]

Updated with new patch to thread this case through InstanceFieldExpr.

Comment by Andy Fingerhut [ 28/Feb/14 6:02 AM ]

A patch for this ticket has been committed as part of Clojure 1.6.0-beta2: https://github.com/clojure/clojure/commit/5fda6cb262d1807566ecadd3af9aaafb58ee5544

It appears this ticket could be closed now.





[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1362-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.

Comment by Stuart Sierra [ 25/Apr/14 1:05 PM ]

Screened.





[CLJ-1359] Fix changelog typos for 1.6 Created: 18/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1359.patch    
Patch: Code
Approval: Ok

 Description   

Some reported problems in the 1.6 changelog:

1) two different issues are both called CLJ-935
2) two issues that are probably different are both called CLJ-1328
3) "Make range consistently return () with a step of 0." This is slightly incorrect. Range now consistently returns an infinite sequence of start with a 0 step.

Patch: clj-1359.patch - updated for these issues, may want to hold this and update for any post-beta1 changes too.






[CLJ-1357] It's a small typo in the gen-class doc-string Created: 17/Feb/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File CLJ-1357-its-typo.patch    
Patch: Code
Approval: Ok

 Description   

"It's" should be "its" (possessive) in "It's return value is ignored."

Screened by: Alex Miller






[CLJ-1356] clojure.org/agents calls out deprecated funcs Created: 17/Feb/14  Updated: 17/Feb/14  Resolved: 17/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Ryan Macy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: agents, documentation, website


 Description   

""If any exceptions are thrown by an action function, no nested dispatches will occur, and the exception will be cached in the Agent itself. When an Agent has errors cached, any subsequent interactions will immediately throw an exception, until the agent's errors are cleared. Agent errors can be examined with agent-errors and cleared with clear-agent-errors.""

While it is true and those functions will do what it describes, they are listed as deprecated in the docs. Should we update this paragraph to reflect usage of `agent-error` and `restart-agent` instead?



 Comments   
Comment by Ryan Macy [ 17/Feb/14 11:38 AM ]

I hope I put this in the right place!

Comment by Alex Miller [ 17/Feb/14 12:32 PM ]

Yep, thanks!

Comment by Alex Miller [ 17/Feb/14 12:40 PM ]

Fixed.





[CLJ-1355] Restore symbol and keyword hashCode to avoid breaking compiled case expressions Created: 17/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojure 1.6.0-beta1


Attachments: File clj-1355-cached.diff     Text File clj-1355-v2.patch    
Patch: Code
Approval: Ok

 Description   

case expressions compiled in Clojure 1.5 are broken if run with Clojure 1.6 where hashCode behavior has diverged from hasheq. In particular, Symbol and Keyword fall into this category.

Approach: Cache both hashCode (with 1.5 calculation) and hasheq (new 1.6 calculation) in Symbol and just hasheq in Keyword. In 1.5, these were the same and case expressions compiled with 1.5 will store the old hash calculation. In 1.6, the hashCode of an expression will be used for comparison.

I tested this by AOT compiling a project in clojure 1.5.1 with this function:

(defn check [v]
  (case v
    :k "keyword match"
    'k "symbol match"
    "k" "string match"
    "no match"))

I verified that (check :k) and (check 'v) incorrectly returned "no match" on Clojure 1.6.0-beta1. I then verified that they returned "keyword match" and "symbol match" respectively on Clojure 1.6.0-master with this patch applied.

Patch: clj-1355-v2.patch



 Comments   
Comment by Alex Miller [ 17/Feb/14 9:38 AM ]

Add patch that caches a new hash field for both Symbol and Keyword that retains Clojure 1.5 computations.

Comment by Alex Miller [ 17/Feb/14 10:42 AM ]

There is a concern here that we are adding a new int field to every Symbol and Keyword (and keyword holds a symbol, so it's really 2 for each keyword).

Comment by Rich Hickey [ 20/Feb/14 7:27 PM ]

I don't think we need to cache in keyword, it's just an add

Comment by Alex Miller [ 20/Feb/14 9:24 PM ]

Updated patch to only cache hashCode in symbol and compute in Keyword.





[CLJ-1354] Make the class APersistentVector.SubVector public Created: 17/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch    
Patch: Code
Approval: Ok

 Description   

The patch marks APersistentVector.SubVector public so that it can be used as a type hint for reflection-free access to subvec internals. I missed this in CLJ-1150.

core.rrb-vector needs access to the internals of the built-in vector types in order to support their efficient concatenation and (true, RRB-style) slicing.

Patch: 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 17/Feb/14 7:30 AM ]

This is the exact spot where I'm trying to get at SubVector internals in core.rrb-vector:

https://github.com/clojure/core.rrb-vector/blob/core.rrb-vector-0.0.10/src/main/clojure/clojure/core/rrb_vector/rrbt.clj#L976

With 1.6.0-alpha3, {{(fv/catvec (subvec [0 1 2 3] 1 2) [:foo])}} results in IllegalAccessError tried to access class clojure.lang.APersistentVector$SubVector from class clojure.core.rrb_vector.rrbt$eval2476$fn__2477 clojure.core.rrb-vector.rrbt/eval2476/fn--2477 (rrbt.clj:978). With this patch applied, it works as expected, returning [1 :foo].





[CLJ-1353] Prevent test app from appearing in Mac OS X dock Created: 16/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Mac OS X


Attachments: Text File CLJ-1353-no-mac-dock.patch     Text File CLJ-1353-v2.patch     Text File clj-1353-v3.patch     Text File clj-1353-v4.patch    
Patch: Code
Approval: Ok

 Description   

During a local ant build of Clojure (tested with master after release of 1.6.0-beta1), the script/run_test.clj is executed. As a side-effect on the Mac, the Java coffee cup app icon is placed in the Dock, and the test app becomes the active application on the desktop. This is slightly annoying.

Even with this property set, activation of awt causes focus to switch temporarily then switch back (at least on Mac).

Solution: Set the following properties during the build:

java.awt.headless=true

Patch: clj-1353-v4.patch



 Comments   
Comment by Steve Miner [ 16/Feb/14 1:39 PM ]

CLJ-1353-no-mac-dock.patch adds a line to script/run_tests.clj to set the apple.awt.UIElement property. This prevents the test app from appearing in the Dock on Mac OS X.

Comment by Steve Miner [ 16/Feb/14 2:18 PM ]

CLJ-1349 might rearrange the affected source, which would force an update to this patch. Still just a one-liner so maybe it could be added to the patch for CLJ-1349.

Comment by Alex Miller [ 16/Feb/14 5:20 PM ]

I also find this highly annoying.

Comment by Andy Fingerhut [ 17/Feb/14 11:21 AM ]

Patch CLJ-1353-v2.patch is identical to Steve Miner's CLJ-1353-no-mac-dock.patch, except it adds another line to build.xml to set the property there, too. At least on my Mac systems, an icon appears in the dock during compilation, not only during testing, and this added line prevents that. Keeps Steve as the patch author.

Comment by Andy Fingerhut [ 17/Feb/14 11:22 AM ]

I tested CLJ-1353-v2.patch on a Linux system, too, and at least the messages that appear on the console during the execution of "ant" are identical with and without this patch, so no extra warnings appear due to these extra properties being set that are likely ignored by the JVM there.

Comment by Steve Miner [ 17/Feb/14 1:45 PM ]

Adding the sysproperty setting to build.xml sounds like a good idea. Thanks.

Comment by Alex Miller [ 18/Feb/14 1:42 PM ]

I found that even with this property, I still see focus change, then come back, during the build due to the activation of awt. Adding the java.awt.headless=true property made that stop. I updated the patch in both locations and now on Mac focus is never stolen during the build.

FYI: If you see the Java "Allow incoming network connections?" dialog on Mac during the tests in response to creating the Sockets in test/clojure/test_clojure/java/io.clj (test-socket-iofactory), this procedure makes that stop:

http://techblog.willshouse.com/2012/10/17/how-to-allow-java-in-the-firewall-on-os-x-mountain-lion/

Beware tracking down the correct version of Java (for example the 1.6 version) instead of the easier to find 1.7 version - the permissions are separate for each version.

Comment by Andy Fingerhut [ 24/Feb/14 2:35 PM ]

In my testing, the addition of the java.awt.headless=true properties in both build.xml and src/script/run_tests.clj was sufficient to avoid the additional icon appearing, and also avoiding any change of focus. Setting apple.awt.UIElement=true appears to be unnecessary (but harmless).

Comment by Steve Miner [ 24/Feb/14 3:28 PM ]

Yes, it seems that java.awt.headless=true is a better, more general solution for the build process. I think apple.awt.UIElement would be appropriate if you actually needed AWT for user interaction but didn't want the dock icon.

Comment by Alex Miller [ 25/Feb/14 11:33 AM ]

Added v4 patch that only sets java.awt.headless=true and drops the apple property.





[CLJ-1352] clojure.test/test-vars runs :each fixtures for vars without :test metadata Created: 14/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File tcrawley-fixtures-with-non-test-vars-2014-02-14.diff    
Patch: Code and Test
Approval: Ok

 Description   

The patch for CLJ-866 introduced a bug with :each fixtures and non-test vars: the fixtures are invoked for every var, not just ones with :test metadata.

Patch: tcrawley-fixtures-with-non-test-vars-2014-02-14.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 24/Feb/14 2:37 PM ]

The patch for this ticket has been committed: https://github.com/clojure/clojure/commit/919a7100ddf327d73bc2d50d9ee1411d4a0e8921

but the ticket has not yet been closed.

Comment by Alex Miller [ 24/Feb/14 3:09 PM ]

yeah, I noticed that too. I was going to mention it to Stu the next time we talked.





[CLJ-1350] (/ 1 3) returns Ratio 31/3 Created: 14/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Justin Hanekom Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

openSuSE 13.1



 Description   

(/ 1 3) incorrectly returns the Ratio 31/3. Other numbers, such as (/ 1 4), work as expected. This could be worked around by using Java interop, but I don't think / it is functioning correctly in this case.



 Comments   
Comment by Justin Hanekom [ 14/Feb/14 1:25 AM ]

$ lein version
Leiningen 2.3.4 on Java 1.7.0_51 OpenJDK 64-Bit Server VM

Comment by Nicola Mometto [ 14/Feb/14 5:35 AM ]

I cannot reproduce this on clojure 1.5.1 or 1.6.0-master-SNAPSHOT

Comment by Alex Miller [ 14/Feb/14 8:14 AM ]

I also could not reproduce on 1.5 or 1.6. Please provide more information on your Clojure environment ({\*clojure-version\*} and also verify that you're not seeing printing obscuring your repl output or something.

user=> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user=> (def x (/ 1 3))
#'user/x
user=> (numerator x)
1
user=> (denominator x)
3
user=> x
1/3
Comment by Justin Hanekom [ 14/Feb/14 12:01 PM ]

Today I'm unable to reproduce this behavior, although yesterday I could!? I'm so embarrassed :-*>

Thanks for closing.





[CLJ-1348] Add functions for external collection hashing Created: 10/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1348-1.patch     Text File clj-1348-2.patch     Text File clj-1348-3.patch    
Patch: Code
Approval: Ok

 Description   

External collections wishing to implement hasheq appropriately must follow the advice at http://clojure.org/data_structures#hash. To simplify the implementation (and avoid unwanted dependencies on the internal Murmur3 class), add two new functions hash-ordered-coll and hash-unordered-coll that provide a proper collection hasheq over entire collections.

Patch: clj-1348-3.patch (fixes [k v])



 Comments   
Comment by Alex Miller [ 10/Feb/14 9:27 AM ]

Added patch. Will need to be refreshed once other patches go in.

Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Rich Hickey [ 12/Feb/14 10:53 AM ]

[k,v] => [k v]

Comment by Alex Miller [ 12/Feb/14 11:46 AM ]

New patch fixing [k v].





[CLJ-1345] Add 1.6 beta changelog updates Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1345-2.patch     Text File clj-1345.patch    
Patch: Code
Approval: Ok

 Description   

Update changelog for 1.6 beta.

Patch: clj-1345-2.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Alex Miller [ 12/Feb/14 12:47 PM ]

Updated patch to fix if-some and when-some definitions.





[CLJ-1344] defrecord still uses old hashing algorithm Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File clj-1344-1.patch    
Patch: Code
Approval: Ok

 Description   

defrecord implements hasheq by calling clojure.lang.APersistentMap/mapHasheq. mapHasheq uses the old map hash calculation instead of the new one. At least one external collection (data.avl) also calls this function. It should be updated to match the new hasheq calculations.

I considered changing defrecord to call Murmur3 directly, but this would create a case where the generated class does not work with older Clojure runtimes so I left it at calling mapHasheq instead.

Patch: clj-1344-1.patch



 Comments   
Comment by Alex Miller [ 07/Feb/14 1:33 PM ]

Attached patch to make mapHasheq use new hash map calculation.

Comment by Alex Miller [ 10/Feb/14 4:01 PM ]

oops





[CLJ-1343] Add some?, when-some, if-some for (not (nil? x)) conditions Created: 07/Feb/14  Updated: 15/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1343-1.patch     Text File clj-1343-2.patch     Text File clj-1343-3.patch     Text File clj-1343-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

Sometimes it is useful to have a form for non-nil conditions (as opposed to the existing logical true conditions).
Three additions to support this case:

  • some? - same as (not (nil? x))
  • if-some - like if-let, but checks (some? test) instead of test
  • when-some - like when-let, but checks (some? test) instead of test

Patch: clj-1343-4.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Tassilo Horn [ 11/Feb/14 2:32 AM ]

At least to me, the name `some?` doesn't convey the same information as "not nil", so I'd rather prefer a more explicit name like `non-nil?`.

Also, I'm not convinced of the benefit of something like `(when-some x ...)` compared to `(when-not (nil? x) ...)`. A little shorter and one pair of parens less, but IMHO not as clear.

Comment by Jozef Wagner [ 11/Feb/14 2:59 AM ]

In my opinion, some? should be defined as (not (empty? coll)), and used as in "are there 'some' items in this collection?". This will also play nicely with some, which also takes collection as an argument.

Comment by Tassilo Horn [ 12/Feb/14 1:02 AM ]

Jozef, for that purpose, you'd use `seq`. Actually, the definition of `empty?` is `(not (seq coll))`, so your suggestion would boil down to `some?` being `(not (not (seq coll)))`.

Comment by Rich Hickey [ 12/Feb/14 10:56 AM ]

if-some and when-some are supposed to be like if-let and when-let respectively. Changelog will need updating as well

Comment by Alex Miller [ 12/Feb/14 12:38 PM ]

Updated patch to make if-some and when-some similar to if-let and when-let.

Comment by Alex Miller [ 14/Feb/14 10:01 AM ]

New patch that does not use "some?" in if-some and when-some.

Comment by Alex Miller [ 14/Feb/14 10:39 AM ]

New patch that adjusts when-some impl.

Comment by Kyle Kingsbury [ 15/Feb/14 1:04 PM ]

I'd like to echo Jozef Wagner's and Steve Losh's confusion here.

```
user=> (some odd? [1 2 3])
true
user=> (some? odd? [1 2 3])

ArityException Wrong number of args (2) passed to: user$some-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
```

I might expect (some?) to behave like (some), except returning a boolean instead of a logically true value, but this is clearly not the case. In no other case in the stdlib can I think of two functions which differ only by punctuation yet have completely different semantics.

```
user=> (some? [])
true
```

Given (some)'s association with sequences, I might interpret (some?) to mean "are there some elements here?"; but that's definitely wrong. Given we have (not=), (not-any?), (not-empty), and (not-every?), can we please name this function (not-nil?)? It's only three characters, but makes the interpretation unambiguously clear.

```
user=> (def x nil)
#'user/x
user=> (def y nil)
#'user/y
user=> (some? [x y])
true
user=> (when-some [x y] :i-expect-true)
nil
```

The fact that (when-some) and (if-some) behave like let bindings is, erm, quite surprising to me. The other binding conditionals have -let in their name; perhaps it would be appropriate to use -let here as well?

For that matter, is this use case all that common? I think I reach specifically for a nil? test fewer than 1 in 20 conditionals--in those cases, why not just write

```
(when-let [x (not-nil? y)]
...)
```

instead of

```
(when-some [x y]
...)
```

I'm just not convinced that this pattern is common enough to warrant the confusion of (when-some) having nothing to do with (when (some ...)), haha. What do y'all think? Have I missed some symmetry between (some?) and (some) that helps this all make sense?

Comment by Alex Miller [ 15/Feb/14 4:36 PM ]

Summarizing comments here, mailing list, Twitter, etc:

  • some uses a truthy comparison. some->, some->> use a not nil comparison. This difference existed in 1.5 some?/if-some/when-some follow the latter. This split is unfortunate, but existed before this addition.
  • not-nil?, non-nil?, nnil?, exists?, and all other alternatives I've seen mentioned were considered as options before the existing names were chosen by Rich. Many people have expressed negative feedback about the name choices and I will channel that to Rich for consideration, but ultimately the choice is his.
  • if-some and when-some are likely more useful than some?. In particular, it is commonly needed when reading from core.async channels where nil is a special value (but false is not).
(go
  (if-some [v (<! c)]
    ...))




[CLJ-1339] Empty primitive vectors throw NPE on .equals with non-vector sequential types Created: 04/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1339.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors have several equality cases. In the case where the compared value is not a vector or random access collection but is a sequential or list, an empty primitive vector will throw an NPE:

user> (.equals (vector-of :long) [])   ;; ok
true
user> (.equals (vector-of :long) '())  ;; broken
NullPointerException   clojure.core.Vec (gvec.clj:135)

Cause: In this case of the primitive vector equals() method, seq is called on itself, then .equals() is invoked on the result. seq will return null for an empty primitive vector, causing an NPE.

Solution: Check for this condition and compare with (nil? (seq o)) on the other object.

Patch: CLJ-1339.patch






[CLJ-1338] New Murmur3 class is not public Created: 04/Feb/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1338.patch    
Patch: Code
Approval: Ok

 Description   

The new Murmur3 class added for hashing is not public, which is problematic for code that needs to call it in several other tickets. To separate out this overlapping change, I have provided it here by itself.






[CLJ-1336] Allow external collections to use standard collection hashing Created: 31/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1336-1.patch     Text File clj-1336-2.patch     Text File clj-1336-3.patch     Text File clj-1336-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

With the change in new hashing algorithms in 1.6, we need to provide a public hook for collections implemented outside of core to participate in the same hash mixing behavior as core collections.

Patch: clj-1336-4.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1335, CLJ-1331



 Comments   
Comment by Alex Miller [ 04/Feb/14 10:42 AM ]

Updated patch for a couple issues. However, in testing the use of this I discovered that the hash-basis must be an int and the basis accumulation must be based on int-accumulation with int-overflow, so it is not possible to do this in pure Clojure so this function is not currently useful.

I think the best solution would be to provide functions that encapsulate the ordered and unordered algorithms (Murmur3/hashOrdered and Murmur3/hashUnordered basically) such that external collections can implement hasheq correctly and with good performance.

Comment by Alex Miller [ 04/Feb/14 2:45 PM ]

Add new patch that makes Murmur3 class public so it will work for users of mix-collection-hash. Also adds generative tests for comparing the external collection hashing algorithm with hashes produced by internal ordered and unordered collections. These tests currently fail due to CLJ-1335 (empty list and empty lazy seq return wrong hash code).





[CLJ-1335] PersistentList$EmptyList and empty LazySeq still returns old value for hasheq Created: 30/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1335-v1.diff     Text File clj-1335-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

After late Jan 2014 changes to hash functions, PersistentList$EmptyList and (lazy-seq) were left behind:

user=> (= '() (lazy-seq) [])
true
user=> (map hash ['() (lazy-seq) []])
(1 1 -2017569654)
user=> (map class ['() (lazy-seq) []])
(clojure.lang.PersistentList$EmptyList clojure.lang.LazySeq clojure.lang.PersistentVector)

PersistentQueue/EMPTY was updated, so should not need any change.

Solution: Update LazySeq.hasheq() and make EmptyList implement IHashEq. EmptyList now creates a static constant for the hash value of an empty ordered collection and returns the constant for hasheq. An alternative would be to have Murmur3 have this constant instead.

Patch: clj-1335-v2.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1331 (must be applied first)

Patch:



 Comments   
Comment by Andy Fingerhut [ 30/Jan/14 6:33 PM ]

Patch clj-1335-v1.diff adds tests that assume the patch clj-1331-v1.diff on ticket CLJ-1331 have already been committed. If it is desired to combine these into one patch, or commit this one without that one, I can eliminate that dependency.

Makes PersistentList$EmptyList implement IHashEq interface with a straightforward implementation of hasheq(), comments out empty LazySeq special case check that caused it to return old hash value, and fixes a NullPointerException for primitive vectors discovered by the new tests added.





[CLJ-1334] Improve performance of the bean function with caching Created: 30/Jan/14  Updated: 31/Jan/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Ron Pressler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: performance


 Description   

The bean function is a very useful Java interop feature that provides a read-only view of a Java Bean as a Clojure map.
As it stands, the function performs introspection on the bean's class whenever the function is called. We can, however, cache the mapping from keywords to getters using JDK 7's handy ClassValue. The proposed function will look like this:

(def ^:private ^java.lang.ClassValue bean-class-value
(proxy [java.lang.ClassValue]
[]
(computeValue [c]
(reduce1 (fn [m ^java.beans.PropertyDescriptor pd]
(let [name (. pd (getName))
method (. pd (getReadMethod))
type (.getPropertyType pd)]
(if (and method (zero? (alength (. method (getParameterTypes)))))
(assoc m (keyword name) (fn [x] (clojure.lang.Reflector/prepRet type (. method (invoke x nil)))))
m)))
{}
(seq (.. java.beans.Introspector
(getBeanInfo c)
(getPropertyDescriptors)))))))

(defn bean
"Takes a Java object and returns a read-only implementation of the
map abstraction based upon its JavaBean properties."
{:added "1.0"}
[^Object x]
(let [c (. x (getClass))
pmap (.get bean-class-value c)
v (fn [k] ((pmap k) x))
snapshot (fn []
(reduce1 (fn [m e]
(assoc m (key e) ((val e) x)))
{} (seq pmap)))]
(proxy [clojure.lang.APersistentMap]
[]
(containsKey [k] (contains? pmap k))
(entryAt [k] (when (contains? pmap k) (new clojure.lang.MapEntry k (v k))))
(valAt ([k] (when (contains? pmap k) (v k)))
([k default] (if (contains? pmap k) (v k) default)))
(cons [m] (conj (snapshot) m))
(count [] (count pmap))
(assoc [k v] (assoc (snapshot) k v))
(without [k] (dissoc (snapshot) k))
(seq [] ((fn thisfn [plseq]
(lazy-seq
(when-let [pseq (seq plseq)]
(cons (new clojure.lang.MapEntry (first pseq) (v (first pseq)))
(thisfn (rest pseq)))))) (keys pmap))))))



 Comments   
Comment by Jozef Wagner [ 30/Jan/14 9:25 AM ]

associated discussion

Comment by Alex Miller [ 31/Jan/14 8:45 AM ]

Curious if anyone is using this inside production code? I use it at the REPL when exploring Java stuff sometimes but have never used it inside my actual code.

Comment by Stuart Halloway [ 31/Jan/14 12:28 PM ]

This is a cool idea, but doesn't need to be in core. How about https://github.com/clojure/java.data ?

Comment by Ron Pressler [ 31/Jan/14 12:48 PM ]

This might be good for java.data, and I'm certainly not saying it should be in the core except for the fact that it already is. This is a proposal for a performance improvement of a core function.





[CLJ-1331] Primitive vectors should use new hash Created: 29/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1331-v1.diff     Text File clj-1331-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors created via vector-of still use Java hashCode for hasheq.

Solution: Make primitive vectors implement IHashEq and call Murmur3.hashOrdered().

Patch: clj-1331-v2.patch
Depends on: CLJ-1338 (must be applied first)



 Comments   
Comment by Andy Fingerhut [ 29/Jan/14 6:03 PM ]

Patch clj-1331-v1.diff is one way to change primitive vectors to use Murmur3 hash.





[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 8
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1093-v3-no-locals-improv.patch     Text File 0001-CLJ-1093-v3.patch     File demo1.clj    
Patch: Code
Approval: Ok

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Impact: this affects apps like Cursive, which has been using a patched version of Clojure to get around this issue for quite a while.

Demonstration script: demo1.clj

Patch: 0001-CLJ-1093-v3.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.
The patch contains an additional enhancement to include the name of the local binding in the class name.

Comparison between pre and post patch naming scheme (N denotes unique number):

code before after note
(defn a []) user$a user$a same
(fn []) user$evalN$fn__N user$evalN$fn__N same
(fn a []) user$evalN$a__N user$evaN$a__N same
(let [a (fn [])] a) user$evalN$a__N user$evalN$a__N same
(let [a (fn x [])] a) user$eval1N$x__N user$evalN$a_x_N IMPROVED - contains local binding name
(def a (fn [])) user$a user$a same
(def a (fn x [])) user$x user$a_x_N FIXED conflict with (defn x [])
(def ^{:foo (fn [])} a) user$fn__N user$fn__N same
(def ^{:foo (fn a [])} a) user$a user$a__N FIXED conflict with (defn a [])
(def a (fn [] (fn []))) user$a$fn__N user$a$fn__N same
(def a (fn [] (fn x []))) user$a$x__N user$a$x__N same

See also: This patch also fixes the issue reported in CLJ-1227.

Screened by: Alex Miller - I am not sure whether the local binding name enhancement is worth doing. It improves debugging of which anonymous class you're talking about but has the downsides of increasing class name (and file name) length.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Comment by Stuart Halloway [ 27/Jun/14 2:17 PM ]

incomplete pending the answers to Alex Miller's questions in the comments

Comment by Nicola Mometto [ 27/Jun/14 3:20 PM ]

I believe I already answered his questions, I'll try to be a bit more explicit:
I tracked the relevant commit from Rich which added the dynamic naming behaviour https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28#diff-f17f860d14163523f1e1308ece478ddbL3081 which clearly shows that this bug was present since then so.

Regarding redefinitions or recursive functions, both of those operations never take in account the generated fn name so they are unaffected.

Comment by Alex Miller [ 12/Sep/14 4:32 PM ]

Summarizing some cases here from before/after the patch:

1) top-level fn (always has name)
	1.6 - namespace$name
	patch - namespace$name
2) non-top-level fn with name
	1.6 - namespace$name (collides with prior case)
	patch - namespace$topname__x__name  	<-- CHANGED
3) anonymous fn (no name)
	1.6 - namespace$name$fn__x
	patch - namespace$name$fn__x
4) top-level anonymous fn (no name, not at all useful :)
	1.6 - namespace$fn__x
	patch - namespace$fn__x

The key problem is that the first 2 cases produce the identical class name on 1.6. The patch alters the non-top-level named fn so there is no conflict.

Prior to the referenced old commit, I believe cases 1 and 2 would both produce namespace$name__x (where x is unique) so they would not collide. The change was made to prevent the top-level name from changing ("don't append numbers on top-level fn class names"). While the similar change was made on non-top-level fn names, I do not think it needed to be.

I've thought through (and tried) a bunch of the implications of this with the help of Nicola's comments above and I do not see an issue with any of the things I've considered. From a binary compatibility point of view with existing AOT code, old code compiled together should be self-consistent and continue to work. New compiled code will also be consistent. I can't think of a way that new code would expect to know the old name of a non-top-level function such that there could be an issue.

One question - why change the code such that the new class name is namespace$name$topname__x__name instead of namespace$name$topname_name__x (or something else?). And relatedly, while the diff is small, could we refactor a couple more lines to make the intent and cases clearer?

I am 90% ok with this patch but want a little thought into that question before I mark screened.

Comment by Nicola Mometto [ 12/Sep/14 4:47 PM ]

Alex, the attached patch munges into ns$topname__name__x, not into ns$topname__x__name.

Comment by Nicola Mometto [ 12/Sep/14 5:22 PM ]

The attached patch 0001-Fix-CLJ-1330refactored.patch contains the same fix from 0001-FixCLJ-1330-make-top-level-named-functions-classnam.patch but also refactors the code that deals with fn name munging

Comment by Alex Miller [ 12/Sep/14 6:22 PM ]

Hmmm.. I will double-check. That's not why I recall seeing when I did AOT.

Comment by Nicola Mometto [ 12/Sep/14 7:26 PM ]

New patch 0001-CLJ-1093-v2.patch improves the fn naming scheme a lot.
I've threw together a number of test cases that show the improvement + bug fixes:

user=> (fn [])
;; pre:
#<user$eval1$fn__2 user$eval1$fn__2@4e13aa4e>
;; post: (no change)
#<user$eval1$fn__3 user$eval1$fn__3@3c92218c>
user=> (fn a [])
;; pre:
#<user$eval5$a__6 user$eval5$a__6@6946a317>
;; post: (no change)
#<user$eval6$a__8 user$eval6$a__8@6f85c59c>
user=> (let [a (fn [])] a)
;; pre:
#<user$eval9$a__10 user$eval9$a__10@15fdf894>
;; post: (no change)
#<user$eval11$a__13 user$eval11$a__13@4d051922>
user=> (let [a (fn x [])] a)
;; pre: (only contains the name of the fn)
#<user$eval17$x__18 user$eval17$x__18@7f0cd67f>
;; post: (contains the name of the local aswell as the name of the fn
#<user$eval21$a__x__23 user$eval21$a__x__23@528ef256>
user=> (def a (fn [])) a
#'user/a
;; pre:
#<user$a user$a@33e1ccbc>
;; post: (no change)
#<user$a user$a@6bef63f9>
user=> (def a (fn x [])) a
#'user/a
;; pre: (BUG!)
#<user$x user$x@59a04a1b> 
;; post: (bug fixed)
#<user$a__x__28 user$a__x__28@5f0bebef>
user=> (def ^{:foo (fn [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre:
#<user$fn__23 user$fn__23@d9c21c6>
;; post: (no change)
#<user$fn__30 user$fn__30@4cf0f2eb>
user=> (def ^{:foo (fn a [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre: (BUG!)
#<user$a user$a@420dd874>
;; post: (bug fixed)
#<user$a__35 user$a__35@37ff95a9>
user=> (def a (fn [] (fn []))) (a)
#'user/a
;; pre:
#<user$a$fn__30 user$a$fn__30@6f57be76>
;; post: (no change)
#<user$a$fn__41 user$a$fn__41@fd34eac>
user=> (def a (fn [] (fn x []))) (a)
#'user/a
;; pre:
#<user$a$x__35 user$a$x__35@79930089>
;; post: (no change)
#<user$a$x__48 user$a$x__48@6fc334de>
user=> (let [x (fn [])] (def a (fn [] x))) a (a)
#'user/a
;; pre:
#<user$eval40$a__43 user$eval40$a__43@6db1694e>
#<user$eval40$x__41 user$eval40$x__41@20bd16bb>
;; post (no change)
#<user$eval54$a__58 user$eval54$a__58@7c721de>
#<user$eval54$x__56 user$eval54$x__56@43f7b41b>
user=> (let [x (fn a [])] (def a (fn [] x))) (a)
#'user/a
;; pre: (the local binding name doesn't appear in the class name)
#<user$eval48$a__49 user$eval48$a__49@75d6d1d4>
;; post: (the local binding name is included in the class name)
#<user$eval64$x__a__66 user$eval64$x__a__66@460d4>

As you can see, this last patch not only fixes the two bugs, but also improves fn naming in let contexts by preserving the name of the local binding in the class name, this I believe will be a great improvement in the understandability of stacktraces.

Comment by Alex Miller [ 25/Sep/14 7:00 AM ]

The patch should be changed to not create suffix if it's not going to be used. Please update the patch to inline that into each branch name = nm.name + "__" + RT.nextID();.

I am unsure whether the "enhancement" part of this patch goes too far. I think it does provide some improvements in debugging but those seem small to me. I am somewhat concerned about greatly increasing the name of the class for nested locals thus making it harder to read stack traces. There is a large limit to class name size of 16 bits (what you can put in the constant table) but class names also map to file names and there have historically been issues on some older Windows architectures with file size limits - we are increasing the risk of running into problems with this. Small risks. I am ok with passing this on to Rich though and he can decide whether to kick that part back or not.

Comment by Nicola Mometto [ 25/Sep/14 7:08 AM ]

0001-CLJ-1093-v3.patch is identical to 0001-CLJ-1093-v2.patch except it doesn't call RT.nextID() when not necessary, as per Alex's request

Alex, if this is ok please change the "Patch:" field in the description, I won't do that myself since this ticket is now screened

Comment by Nicola Mometto [ 06/Oct/14 11:54 AM ]

Addressing the screening comment by Alex Miller, I've attached an alternative patch "0001-CLJ-1093v3-no-locals-improv.patch" which is identical to "0001CLJ-1093-v3.patch" except it doesn't include the local binding name enhancement, so that it can be picked in case Rich decides that that improvement is out of scope for this ticket.





[CLJ-1328] Make some Clojure tests independent of hash function used Created: 20/Jan/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1328-v3.diff     File clj-1328-v4.diff    
Patch: Code and Test
Approval: Ok

 Description   

The most interesting failures with the new hash function are probably the 3 deftest's in multimethods.clj that define the same multimethod name 'simple', and thus whether they pass or fail depends upon the order that they are executed. They are currently executed in an order that allows them to pass. Found this while testing murmurHash3 changes to Clojure, which caused the deftest's to execute in a different order and fail.

Simplest way to eliminate this dependency on order is to make the multimethod names unique in each test, so none of them depends upon state left behind by the others.



 Comments   
Comment by Andy Fingerhut [ 20/Jan/14 1:18 PM ]

Patch clj-1328-v1.diff makes all defmulti names unique in multimethods.clj, so that no deftest result depends upon state left behind by another.

Comment by Andy Fingerhut [ 29/Jan/14 8:11 PM ]

Updates some, but not all, tests that were recently modified or disabled due to change in hash function.

Comment by Andy Fingerhut [ 29/Jan/14 10:52 PM ]

Updates one more test than the previous patch.

Comment by Andy Fingerhut [ 31/Jan/14 3:43 PM ]

clj-1328-v4.diff is identical to clj-1328-v3.diff, except it adds a comment explaining why the case hash collision tests don't need to change much, and it puts in a couple of missing (is ...) around some equality tests.





[CLJ-1325] Report warnings if *unchecked-math* and boxing happens Created: 16/Jan/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt     Text File clj-1325.patch     Text File clj-1325-v2.patch     Text File clj-1325-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. The proposed enhancement here is to emit new warnings if *unchecked-math* is on and boxed math is occurring.

Approach: In the compiler, when compiling a StaticMethodExpr, if *unchecked-math* is true and the class is clojure.lang.Numbers and one of the parameters of static method is of type java.lang.Object or java.lang.Number, then emit a warning at compile-time.

In addition, there is a new WarnBoxedMath Java annotation - a small number of methods on Numbers with Object parameters use this annotation to indicate that warning should not take place. The same annotation can be (but is not currently) used to mark methods on Numbers without Object/Number params that should warn. See boxedmath.txt for a list of methods and categories.

Patch: clj-1325-v3.patch

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

Comment by Alex Miller [ 15/Apr/14 10:17 AM ]

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.

Comment by Alex Miller [ 14/May/14 2:34 PM ]

Ready for screening.

Comment by Alex Miller [ 16/May/14 11:19 AM ]

clj-1325-v2.patch is identical to last except for a cleaned up the commit message.

Comment by Alex Miller [ 16/May/14 11:51 AM ]

Added v3 patch that just reworks block/indentation style to match surrounding code better.

Comment by Stuart Sierra [ 16/May/14 1:15 PM ]

Screened. Comments:

1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter.

2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is.

3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.





[CLJ-1320] min-key assumes numbers, not comparables. Created: 09/Jan/14  Updated: 09/Jan/14  Resolved: 09/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File min-key.diff    
Patch: Code

 Description   

The min-key function assumes the key-fn will yield a number and thus uses the '<' operator to compare results.
There are cases where one might want to use min-key with comparables instead.

While (first (sort-by key-fn seq)) could also be used, it feels more natural for min-key to use comparables.



 Comments   
Comment by Pierre-Yves Ritschard [ 09/Jan/14 3:18 PM ]

As discussed on the .L, since compare is slower it makes more sense to keep min-key as-is.





[CLJ-1318] Support destructuring maps with namespaced keywords Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1318-1.diff     File clj-1318-2.diff     File clj-1318-3.diff     File clj-1318-4.diff     File clj-1318-5.diff     File clj-1318-6.diff    
Patch: Code and Test
Approval: Ok

 Description   

Current :keys destructuring expects symbols and creates local bindings based on those symbols. This works fine with maps that use non-namespaced keyword keys. This enhancement is to add support for destructuring maps with namespaced keyword keys.

;; typical key destructuring for keyword keys without namespaces
(let [{:keys [a b]} {:a 1 :b 2}] (+ a b))

;; WANT some way to destructure map with namespaced keys
(let [{:keys [????]} {:x/a 1 :y/b 2}] (+ a b))

Approach: Allow keywords (with or without namespaces) in :keys destructuring. Destructure to bindings with the name of the keyword (namespace is ignored).

;; this now works
(let [{:keys [x/a y/b]} {:x/a 1 :y/b 2}] (+ a b))

;; add support for putting keywords into :keys as well to support ::keywords
(let [{:keys [:x/a :y/b]} {:x/a 1 :y/b 2}] (+ a b))
(let [{:keys [::a]} {:user/a 1}] a)

;; syms will also now support namespaced symbols
(let [{:syms [x/a y/b]} {'x/a 1 'y/b 2}] (+ a b))

Patch: clj-1318-6.diff

Screened by: Stuart Sierra. See comments, below.

Doc TODO: Will need to update http://clojure.org/special_forms#binding-forms with new binding form.



 Comments   
Comment by Nicola Mometto [ 06/Jan/14 11:58 AM ]

Why {:keys [:a/b]} and not {:keys [a/b}}?
Also, this should probably be extended to :syms for consistency

Comment by Alex Miller [ 06/Jan/14 12:28 PM ]

Good questions both. For the first question, we want to make locally namespaced keywords (::foo) work and there is no way to say that as a symbol.

I am waiting to hear back from Rich whether support for namespaced :syms is desirable. I think the change to support it is identical to the change to support namespaced keywords as symbols. I'm going to proactively update the patch to support that too.

Comment by Alex Miller [ 06/Jan/14 12:50 PM ]

Added new patch - now supports namespaced symbols or keywords in :keys and namespaced symbols in :syms.

Comment by Rich Hickey [ 06/Jan/14 1:00 PM ]

Should (also) support symbols for names, e.g. {:keys [a/b]}, only limitation is you can't get ns alias resolution. :syms support makes sense, but may seem weird to provide keywords for local names (where it doesn't as much for keywords), but would allow reaching aliases. My preference is no keyword names support for :syms, i.e. {:syms [a/b]} ok, {:syms [:a/b]} not.

Comment by Nicola Mometto [ 06/Jan/14 1:10 PM ]

To me {:syms [:a/b]} doesn't feel any more weird than writing {:keys [:a/b]}.
If this is going to be added, I think it should be consistent for :keys and :syms.
I understand that :syms is rarely used and this should not be an issue realistically, but I would expect everything that works for :keys to work for :syms too and adding only half a feature to :syms might cause unnecessary confusion.

Comment by Nicola Mometto [ 07/Jan/14 2:16 PM ]

With this patch this will now work:

user=> (let [:a/b 1] b)
1

I don't think this is desiderable.

Comment by Alex Miller [ 07/Jan/14 3:52 PM ]

Right, that is a consequence of allowing keywords in the :keys. At a glance this seems hard to address without significant changes unless we catch it prior to processing. Will consider.

Comment by Alex Miller [ 07/Jan/14 4:40 PM ]

Added new patch variant that catches keywords as let binding keys and throws an Exception.

Comment by Alex Miller [ 10/Jan/14 2:24 PM ]

Added one test in -4 showing example of auto-resolved keywords in :keys.

Comment by Stuart Sierra [ 10/Jan/14 3:00 PM ]

Screened. A few comments:

1. The examples in the tests use {:keys (a b)} with lists instead of {:keys [a b]} with vectors. Both forms are accepted both before and after the patch, but the docs at Clojure - special_forms only show vectors.

2. I would like this to work, but it would add some complexity:

(ns com.example.myproject.foo)

  (def data
    {::a 1 ::b 2})

  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
  (ns com.example.myproject.bar
    (:require [com.example.myproject.foo :as foo]))

  ;; I would like this to work:
  (let [{:keys [foo/a foo/b]} foo/data]
    [a b])
  ;;=> [nil nil]

  ;; This is good enough, however:
  (let [{:keys [::foo/a ::foo/b]} foo/data]
    [a b])
  ;;=> [1 2]

3. This doesn't produce an error, which is logically consistent but perhaps not desirable:

(let [{:a ::foo/a} foo/data]
    [a])
Comment by Rich Hickey [ 24/Jan/14 10:11 AM ]

please change the tests to use vectors

Comment by Alex Miller [ 24/Jan/14 10:28 AM ]

Added new -5 diff that uses vectors instead of lists in :keys tests.

Comment by Alex Miller [ 24/Jan/14 11:07 AM ]

And also fixing :syms [] in -6 diff.

Comment by Alex Miller [ 24/Jan/14 11:08 AM ]

Changed examples in description to use [].

Comment by Fogus [ 07/Feb/14 2:23 PM ]

A potential point of confusion here is illustrated by the following:

(let [m {:x/a 1, :y/b 2, :x/b 3000}
        {:keys [x/a y/b x/b]} m]
  (+ a b))

//=> 3

To get the answer 3001 one needs to remove the conflicting binding :y/b. Maybe this is not a big deal, but expect questions for the next 100 years.

Comment by David Nolen [ 23/Feb/14 5:01 PM ]

Ported to ClojureScript with CLJS-745





[CLJ-1315] Don't initialize classes when importing them Created: 28/Dec/13  Updated: 07/Oct/14  Resolved: 07/Oct/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.1, Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: Release 1.7

Type: Enhancement Priority: Critical
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Completed Votes: 9
Labels: aot, compiler, interop

Attachments: Text File 0001-Don-t-initialize-classes-during-import.patch    
Patch: Code
Approval: Ok

 Description   

Problem: When classes are imported in Clojure, the class is loaded using Class.forName(), which causes its static initialisers to be executed. This differs from Java where compilation does not cause classes to be loaded.

Motivation: In many cases when those classes are normally loaded by Java code during execution of a framework of some kind (IntelliJ in my case, and RoboVM is another culprit mentioned in that thread) the initialiser expects some infrastructure to be in place and will fail when it's not. This means that it's impossible to AOT compile namespaces importing these classes, which is a fairly serious limitation.

Approach: Modify ImportExpr to call RT.classForNameNonLoading() instead of Class.forName(), which will load the class but not initialise it. This change causes the Clojure test suite to fail, since clojure.test-clojure.genclass imports a gen-class'ed class which no longer loads its namespace on initialisation. I'm not sure if this is considered an incorrect use of such a class (IIRC with records it's required to import the class and require its namespace), but given that it's in the Clojure test case it's reasonable to assume that this fix would be a breaking change for more code out there. This test failure is also corrected in the attached patch.

Patch: 0001-Don-t-initialize-classes-during-import.patch

Screened by: Alex Miller - I have tested many open source Clojure projects with this change (particularly seeking out large, complicated, or known users of genclass/deftype/etc) and have found no projects adversely impacted. I know that Cursive has been running with this modification for a long time with no known issues. I am ok with unconditionally enabling this change (re the comment below). The impact is described in more detail in the suggested changelog diff in the comments below.

Alternative: This patch enables the change unconditionally, but depending on the extent of breakage it causes, it might need to be enabled with a configuration flag. I propose we make it unconditional in an early 1.7 beta and monitor the fall-out.

Background: This issue has been discussed in the following threads
https://groups.google.com/d/topic/clojure/tWSEsOk_pM4/discussion
https://groups.google.com/forum/#!topic/clojure-dev/qSSI9Z-Thc0



 Comments   
Comment by Alex Miller [ 29/Dec/13 12:23 PM ]

From original post:

This issue was originally reported by Zach Oakes and Colin Fleming and this patch was also tested by Colin.

I'm duplicating here my suggested release notes for this issue, which includes my current thoughts on potential breakage (it's also in the commit message of the patch):

    "import" no longer causes the imported class to be initialized. This
    change better matches Java's import behavior and allows the importing of
    classes that do significant work at initialization time which may fail.
    This semantics change is not expected to effect most code, but certain
    code may have depended on behavior that is no longer true.

    1) importing a Class defined via gen-class no longer causes its defining
    namespace to be loaded, loading is now deferred until first reference. If
    immediate loading of the namespace is needed, "require" it directly.
    2) Some code may have depended on import to initialize the class before it
    was used. It may now be necessary to manually call (Class/forName
    "org.example.Class") when initialization is needed. In most cases, this
    should not be necessary because the Class will be initialized
    automatically before first use.
Comment by Greg Chapman [ 13/May/14 6:25 PM ]

I'm not sure if this should also be fixed, but it would be nice if you could emit the code for a proxy of one of these non-initialized classes without forcing initialization. For example, the following raises an exception (I'm using Java 8):

Clojure 1.6.0
user=> (def cname "javafx.scene.control.ListCell")
#'user/cname
user=> (let [cls (Class/forName cname false (clojure.lang.RT/baseLoader))] (.importClass *ns* cls))
javafx.scene.control.ListCell
user=> (defn fails [] (proxy [ListCell] [] (updateItem [item empty] (proxy-super item empty))))
CompilerException java.lang.ExceptionInInitializerError, compiling:(NO_SOURCE_PATH:3:16)

The exception was ultimately caused by "IllegalStateException Toolkit not initialized", which javafx throws if you attempt to initialize a Control class outside of Application.launch.





[CLJ-1312] clojure.string/split on empty string includes empty string in results Created: 21/Dec/13  Updated: 07/Sep/14  Resolved: 21/Dec/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Russell Dunphy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

Splitting a string using clojure.string/split with an empty regex includes the empty string in the results - is this expected behaviour?

Example:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
user=> (clojure.string/split "abc" #"")
["" "a" "b" "c"]


 Comments   
Comment by Alex Miller [ 21/Dec/13 8:05 AM ]

Yes, I think so. This is a case where Clojure defers to the host (Java) for behavior. I think the way to interpret this is that the empty pattern matches all strings. Split checks left to right whether there is a next chunk of string that matches the pattern. The empty pattern matches at the beginning to a string of length 0. Something like that.

Comment by Mark Engelberg [ 07/Sep/14 12:27 PM ]

This bug is a real problem, because it works differently on Windows than on Linux. On Windows, clojure.string/split behaves exactly as you'd expect:

user=> (clojure.string/split "abc" #"")
["a" "b" "c"]

Only on Linux do you get the strange behavior where the empty string shows up at the beginning of the list.

I recently had a student that got burned by this in some webserver code that relied on splitting using the empty regex. It performed flawlessly on her local Windows machine, but mysteriously broke when she uploaded the uberwar to the cloud. The bug was very difficult to track down.

If this were a bug on both Windows and Linux, at least you could plan around it. But right now, it's an obstacle to Clojure's capability of running consistently across platforms.

Comment by Mark Engelberg [ 07/Sep/14 12:40 PM ]

Upon further research, I've found that this is not a Windows/Linux issue, rather it's a difference between Java 7 and Java 8. On Java 8, splitting with the empty string no longer produces a sequence that begins with an empty string.

As you said before, this is just a gotcha relating to Java, not a Clojure issue.





[CLJ-1310] some-> behaves differently than -> when used with macros Created: 19/Dec/13  Updated: 19/Dec/13  Resolved: 19/Dec/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Chris Perkins Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I stumbled across a behavior of some-> when used with macros, and I'm wondering whether it's expected.

I started with something like this:

(some-> value
foo
bar
quux)

Then I realized that quux might throw an exception, which I want to ignore, so had the possibly misguided idea to do this:

(defmacro catch->nil [& body]
`(try ~@body (catch Exception _# nil)))

(some-> value
foo
bar
quux
catch->nil)

My mental model of some-> is that it should end up wrapping the whole expression in a try-catch, as > does, but that does not happen. some> expands into a `let` instead of a deeply-nested form, so the exception is not caught.

Certainly easy to work around, now that I know about it, but I thought perhaps this was not intended.



 Comments   
Comment by Chris Perkins [ 19/Dec/13 7:34 PM ]

After a little reflection, I realize that my mental model of some-> as "thread-first plus magic fairy-dust" is fatally flawed. I withdraw my objections. Please disregard.

Comment by Alex Miller [ 19/Dec/13 7:50 PM ]

withdrawn by submitter





[CLJ-1307] Type hint remains unqualified, resulting in errors Created: 15/Dec/13  Updated: 15/Dec/13  Resolved: 15/Dec/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

This snippet (even though unsensical) demonstrates the issue:

user=> (ns foo (:import clojure.lang.RT))
nil
foo=> (defn x ^RT [])
#'foo/x
foo=> (ns bar (:use foo))
nil
bar=> (.hashCode (x))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: RT, compiling:(NO_SOURCE_PATH:4:1)

This is because (:tag (meta #'x)) returns the Symbol 'RT, it should either be the Symbol 'clojure.lang.RT or the Class clojure.lang.RT



 Comments   
Comment by Nicola Mometto [ 15/Dec/13 8:43 PM ]

Duplicate: http://dev.clojure.org/jira/browse/CLJ-1232





[CLJ-1306] Cannot reduce over short[] arrays Created: 14/Dec/13  Updated: 14/Dec/13  Resolved: 14/Dec/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug

Attachments: File clj-1306.diff    

 Description   

Reducing over a short array currently causes an error:

(reduce + (seq (short-array 10)))
=> ClassCastException [S cannot be cast to [Ljava.lang.Object; clojure.core.protocols/fn--6037 (protocols.clj:126)

This appears to occur because ArraySeq is assumed by protocols.clj to contain an Object[] array in the ".array" field, when in fact it is a short[] array.

Proposed solution to to create ArraySeq_short (analogous to the other primitive types ArraySeq_long etc.) to handle short arrays.



 Comments   
Comment by Mike Anderson [ 14/Dec/13 7:37 AM ]

Fix for CLJ-1306

Comment by Alex Miller [ 14/Dec/13 8:17 AM ]

This was also discovered in CLJ-1200 and the patch for that issue includes ArraySeq_short as you propose. It is expected that CLJ-1200 will be included in 1.6.

Comment by Mike Anderson [ 14/Dec/13 9:29 AM ]

OK, thanks Alex!

Just to be sure: Can you confirm that a fix will definitely go into 1.6? This is a defect, and as such should have a higher priority than CLJ-1200 (which appears to be presented as an performance enhancement patch).

My patch also includes a regression test which I think could helpfully be included in the test suite.

Comment by Alex Miller [ 14/Dec/13 11:24 AM ]

Yes, I fully expect CLJ-1200 to be included and talked to Rich about it as recently as yesterday. I could split things out of that patch and pull both of these in separately. That would be objectively better but definitely more work to do all the ticket, patch, and screening work so I'd rather not. If you want to attach just the regression test to 1200, I think we could include it that way as 1200 hasn't been screened yet. Stuart Sierra is planning to screen it in the next few days.





[CLJ-1304] Fixed minor typos in documentation and code comments Created: 09/Dec/13  Updated: 04/Feb/14  Resolved: 04/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Vipul A M Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: File clj-1304-v2.diff     File doc-comment-typos.diff    
Patch: Code
Approval: Screened

 Description   

Fixed minor typos in documentation and code comments across multiple files.



 Comments   
Comment by Andy Fingerhut [ 11/Jan/14 2:53 PM ]

Patch clj-1304-v2.diff dated Jan 11, 2014 is identical to Vipul's patch doc-comment-typos.diff dated Dec 9, 2013, except it applies cleanly to latest master. The only changes are that it removes the part of the patch for files in the ASM library, which was updated in a recent commit to Clojure master.

Comment by Alex Miller [ 04/Feb/14 9:21 PM ]

reopen so that I can set the fix version which didn't get set.

Comment by Alex Miller [ 04/Feb/14 9:22 PM ]

re-close now that fix version is set





[CLJ-1303] Remove (apparently) vestigial forward-defs of unquote and unquote-splicing Created: 05/Dec/13  Updated: 01/Feb/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: David Rupp Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement

Attachments: File generalize-unquote.diff    
Patch: Code

 Description   

clojure/core.clj contains forward defs of unquote and unquote-splicing that seem no longer to be necessary. The pull request at https://github.com/clojure/clojure/pull/45 removes this dead code (also attaching a git diff file). Existing tests pass; no new test code necessary.



 Comments   
Comment by Alex Miller [ 05/Dec/13 9:23 PM ]

FYI for future reference, Clojure doesn't accept pull requests. Thanks for the report though!

Comment by David Rupp [ 05/Dec/13 9:31 PM ]

I noticed. That's why I created the JIRA.

Comment by Andy Fingerhut [ 07/Dec/13 10:25 AM ]

David, I do not have any comment on whether this patch will be accepted or not based on the changes it makes, but patches do need to be in a particular format, including the author's name. See instructions for how to create a patch in this format here: http://dev.clojure.org/display/community/Developing+Patches

Comment by David Rupp [ 07/Dec/13 11:26 AM ]

Submitting properly-formatted patch.

Comment by David Rupp [ 09/Dec/13 7:49 AM ]

Replaced references to clojure.core/unquote and .../unquote-splicing,
which are unbound.

The UNQUOTE and UNQUOTE-SPLICING Symbols in LispReader don't really
refer to anything in clojure.core any longer. They're created and
elided by the reader when it encounters their respective (reader)
macro chars.

Comment by Stuart Halloway [ 31/Jan/14 3:26 PM ]

The patch attached here is poorly-suited for screening. It does more than what it says, (e.g. deleting the def of META) without explaining why.

It also removes things that are commented out. Pretty clear that the BDFL likes having those things stick around.

Comment by David Rupp [ 01/Feb/14 6:57 PM ]

META is not used anywhere. I will explain better next time.

Also, DEREF_BANG has been commented out since 2007 (commit 139ddd146f2a272b7ddda397f54b501ff499c643). Figured it was pretty safe to get rid of at this point. My bad.





[CLJ-1302] keys and vals consistency not mentioned in docstring Created: 04/Dec/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: docstring

Attachments: Text File clj-1302-1.patch     Text File clj-1302-2.patch    
Patch: Code
Approval: Ok

 Description   

(keys m) and (vals m) appear to return stuff in a consistent order, so (= m (zipmap (keys m) (vals m))). This consistency is a useful property. The API docs should state whether it is part of the functions' contract.

Patch: clj-1302-2.patch



 Comments   
Comment by Gary Fredericks [ 11/Dec/13 7:18 PM ]

One thing to keep in mind is that the functions can be used on arbitrary instances of java.util.Map, which, aside from being mutable, could hypothetically (though not realistically) generate their entry sets nondeterministically.

I don't know what any of this means about what the docstring should say. It could claim the consistency for clojure's collections at least.

Comment by Andy Fingerhut [ 11/Dec/13 7:44 PM ]

The ticket creator might already realize this, but note that (= m (zipmap (keys m) (vals m))) is guaranteed for Clojure maps, where m is the same identical map, at least by the current implementation. I am not addressing the question whether it is part of the contract, but I think it would be good to make this explicit if it is part of the contract.

The following is not guaranteed for Clojure maps: (= m1 m2) implies that (= (keys m1) (keys m2)).

The set of keys/vals will be equal, but the order of keys/vals could be different for two otherwise equal maps m1, m2.

Comment by Steve Miner [ 27/Dec/13 11:10 AM ]

I think you can depend on a slightly stronger contract: The order of the results from `keys` and `vals` follows the order of the results from `seq`. As with any pure function, `seq` returns consistent results across multiple calls with the same (identical?) map. The order may be arbitrary for a non-sorted map, but it should be consistent.

Some time ago, I looked for this guarantee in the documentation, but I couldn't find it explicitly stated. However, after looking at the implementation, I think it's safe to depend on this invariant.

Comment by Stuart Halloway [ 31/Jan/14 12:48 PM ]

The absence of this property in the docs is correct. You should not rely on this.

Comment by Nicola Mometto [ 31/Jan/14 7:43 PM ]

I have to say this surprises me, I was relying on this undocumented behaviour expecting it to be implicit.

I did a quick search in github and the number of (zipmap (keys m) (do-something (vals m))) is significant, even some experienced clojure developers seem to have given this property for granted (https://groups.google.com/forum/?fromgroups#!topic/clojure/s1sFVF7dAVs).

Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

Comment by Peter Taoussanis [ 01/Feb/14 3:21 AM ]

Big surprise here too. Could someone (Stu?) possibly motivate a little why this couldn't/shouldn't be a contractual property? It seems like it has utility. Perhaps more importantly, it seems to be an intuitively reasonable assumption. That's subjective, sure, but I feel like I've seen this pattern come up quite widely.

Anecdotally, am quite sure I've made use of the assumption before (i.e. that `(keys m)` and `(vals m)` will return sequences as per pair order).

Would need to review code to see how frequently I've made the error.

To clarify: not disagreeing, just want to understand the thought that's gone in.

> Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

That'd be a big help I think. I'd generally take an

Comment by Peter Taoussanis [ 01/Feb/14 3:58 AM ]

End of comment got mangled somehow.

Was just going to point out that I'm a big fan of how cautious + deliberate Clojure's design tends to be. Being hesitant to pick up needless or half-baked contractual obligations, etc. is a huge strength IMO.

Comment by Rich Hickey [ 01/Feb/14 9:36 AM ]

keys order == vals order == seq order

Comment by Alex Miller [ 05/Feb/14 11:25 AM ]

Tweaked doc.





[CLJ-1301] case expression fails to match a BigDecimal Created: 23/Nov/13  Updated: 26/Jan/14  Resolved: 26/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: Compiler

Attachments: Text File case-alt.patch     File clj-1301-1.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

In 1.5.1 (anywhere before the CLJ-1118 patch), this is the behavior on BigDecimal case matching:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "BigDecimal" "none")

In 1.6 the behavior (post CLJ-1118 patch) has changed:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "none" "none")

In 1.6 after CLJ-1118, I expect to see: ("Long" "BigDecimal" "BigDecimal") as they now have the same hash and hasheq.

Cause: The case constants are hashed in the clojure.core/case macro using clojure.core/hash which calls clojure.lang.util/hasheq(). In Compiler.emitExprForHashes(), a call to clojure.lang.Util/hash(). In Clojure 1.5 these hash values are the same (hash of 1.0M == hasheq of 1.0M == 311). In Clojure 1.6, they are different (hash of 1.0M = 311, hasheq of 1.0M = 31).

In any cases where Java's hashCode and Clojure's hasheq return different values, the case statement can fail to do the correct thing.

Approach: Change Compiler.java to use clojure.lang.Util hasheq() to match the case macro use of clojure.core/hash (which calls clojure.lang.Util.hasheq()).

Patch: clj-1301-1.diff

Screened by:



 Comments   
Comment by Andy Fingerhut [ 23/Nov/13 5:00 PM ]

Patch clj-1301-1.diff modifies Compiler.java so that case* statements use hasheq on the test expression value, rather than Java's hashCode. It also adds a test case that currently fails with latest Clojure master, but passes with the patch.

Comment by Andy Fingerhut [ 23/Nov/13 5:01 PM ]