<< Back to previous view

[CLJ-378] Set thread names on agent thread pools Created: 09/Jun/10  Updated: 31/Jul/14  Resolved: 12/Oct/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

It���s a best practice to name the threads in an executor thread pool with a custom ThreadFactory so that the purpose of these threads is clear in thread dumps and other runtime operational tools. By default these threads are currently called something like "pool-%d-thread-%d", and this is what you���ll see for the agent send thread pools.

I created a patch to do this with thread names like:

  • clojure-agent-send-pool-%d ��� should be fixed # of threads
  • clojure-agent-send-off-pool-%d ��� will be added and removed over time

The patch is attached and I have a signed CA.



 Comments   
Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/378
Attachments:
agent-name.diff - https://www.assembla.com/spaces/clojure/documents/bLEf-eC8Sr373-eJe5cbLA/download/bLEf-eC8Sr373-eJe5cbLA
0378-as-git-patch.patch - https://www.assembla.com/spaces/clojure/documents/cUhT1C1uCr35jCeJe5cbLA/download/cUhT1C1uCr35jCeJe5cbLA

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:cUhT1C1uCr35jCeJe5cbLA]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Second patch simply recreates first patch in the correct form. Please follow the instructions at http://clojure.org/patches when creating patches.

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Updating tickets (#276, #280, #378, #437, #448)

Comment by Andy Fingerhut [ 31/Jul/14 10:58 AM ]

Just a historical note for those doing code/ticket archaeology: This ticket appears to have resulted in the git version 351e05a242740cc415524d03e1d424de516eed75 which you can browse here on Github:

https://github.com/clojure/clojure/commit/351e05a242740cc415524d03e1d424de516eed75





[CLJ-698] class accessible from deftype method bodies is not suitable for instance?, ... Created: 28/Dec/10  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: deftype

Approval: Ok

 Description   

Example interaction: http://pastebin.com/cTdUCKfp
Which directly contradicts documentation for deftype

In the method bodies, the (unqualified) name can be used to name the class (for calls to new, instance? etc).



 Comments   
Comment by Stuart Halloway [ 29/Dec/10 12:45 PM ]

The problem occurs in 1.2 but is fixed on master. Leaving in backlog in case we ever cut another 1.2 release--if not, then mark as fixed.

Comment by Alex Miller [ 18/Apr/14 7:32 AM ]

Apparently fixed in 1.3.





[CLJ-691] missing stacktrace confuses REPL error reporting Created: 13/Dec/10  Updated: 21/Mar/14  Resolved: 17/Dec/10

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0691-stacktrace-missing.patch    
Approval: Ok

 Description   

when Java is in a bad mood, exceptions start flying without any accompanying stack traces. This causes the default REPL's error reporting to barf, as it currently assumes that the first stack trace element always exists. You can see this by trying the following snippet at the REPL several times in succession:

(def x (byte-array 100000000))

When Java is somewhat angry, you will see the appropriate

OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1409)

Eventually Java gets angrier and stops providing stack traces, causing an aget error in clojure.main instead.



 Comments   
Comment by Stuart Halloway [ 13/Dec/10 9:46 AM ]

Results with attached patch:

(def x (byte-array 100000000))
=> OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1409)
;; eventually you get:
def x (byte-array 100000000))
=>OutOfMemoryError Java heap space  [trace missing]
Comment by Ivan Kozik [ 21/Mar/14 3:33 AM ]

To spare readers who find this page after Googling "trace missing": don't go on a wild goose chase for an OpenJDK bug that makes stack traces disappear. It's a somewhat-documented HotSpot optimization, as described here: https://github.com/technomancy/leiningen/issues/1025#issuecomment-38253962

Comment by Andy Fingerhut [ 21/Mar/14 1:49 PM ]

See also CLJ-1102 which fixes some other cases where Clojure was throwing new exceptions when incorrectly expecting at least 1 StackTraceElement in an exception's stack trace. That problem existed up through Clojure 1.5.1, but is fixed in Clojure 1.6.0.





[CLJ-693] VerifyError with symbol metadata, macros, and defrecord Created: 16/Dec/10  Updated: 26/Jul/13  Resolved: 17/Dec/10

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Backlog, Release 1.2, Release 1.3
Fix Version/s: Release 1.3

Type: Defect Priority: Major
Reporter: Constantine Vetoshev Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None
Environment:

Affects Clojure from 1.2.0 to 1.3.0-alpha4


Approval: Ok

 Description   

The code below defines a macro wrapper around defrecord. Using it causes a VerifyError:

(defmacro mac1 [name properties] 
  (let [key-info (keyword (first (filter #(meta %) properties)))] 
    (prn key-info) 
    `(defrecord ~name ~properties))) 

(mac1 One [^:key one, two])

Once this happens, the running Clojure process is oddly damaged. Changing the macro to a working version (see below) will not make the sample work with the tagged symbol, almost as if the symbol
'one (below) became corrupt. You either need to start a new REPL, or try to invoke the macro with a different tagged symbol.

The following code does work (note the forced conversion to a string):

(defmacro mac2 [name properties] 
  (let [key-info (keyword (str (first (filter #(meta %) properties))))] 
    (prn key-info) 
    `(defrecord ~name ~properties))) 

(mac2 Two [^:key three, four])


 Comments   
Comment by Rich Hickey [ 17/Dec/10 8:47 AM ]

Stu, can you please verify?

Comment by Stuart Halloway [ 17/Dec/10 1:53 PM ]

Very confusing. Reproduced everything in the report. Moreover, leaving out the call to keyword is enough to avoid the problem. The following variant works fine:

(defmacro mac4 [name properties] 
  (let [key-info (first (filter #(meta %) properties))] 
    (prn key-info) 
    `(defrecord ~name ~properties)))
Comment by Constantine Vetoshev [ 18/Dec/10 1:02 PM ]

Thanks for the fix! Works great.





[CLJ-695] pprint does not respect *print-length* Created: 18/Dec/10  Updated: 01/Mar/13  Resolved: 31/Dec/10

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0695-partial-fix.patch     File pprint-print-length-fix.diff    
Patch: Code and Test
Approval: Ok

 Description   

pprint does not respect print-length – instead it prints ellipses for every element one length is passed (and prints forever on infinite collections).



 Comments   
Comment by Stuart Halloway [ 18/Dec/10 3:14 PM ]

The attached patch is part of a possible fix. Issues to run down:

  • where and how is the return value of write-out used. (The changes I make eliminate this dependency in favor of a direct check of the dynamic variables.
  • how to make sets work? they are in a different code path than the others
Comment by Tom Faulhaber [ 21/Dec/10 2:36 AM ]

Hmm, looks like I broke some logic when I hand unrolled the original cl-format based dispatch to get better performance for lists, vectors, and maps. (Really I shouldn't have to do this, but I need to make cl-format itself generate code rather than threaded functions which are slow and tend to blow your stack. Haven't gotten around to that yet, though so the hand-coded versions are stop-gaps.)

I'm not digging the patch too much, though, for 3 reasons:

1) It breaks sets and arrays, which work in master.
2) It pushes the logic for print-length printing into the dispatch functions (redundantly since there's still logic in write-out). Since these are customizable, it places that load on whoever customizes it.
3) It adds redundant logic in write-out which is the called for every object that the pretty printer deals with.

I'll try to take a stab at a patch that fits a little better with what I'm trying to do in the next couple of days.

Comment by Tom Faulhaber [ 22/Dec/10 3:13 AM ]

This fixes the the issue with hand-rolled dispatch functions and provides a macro to help other users do it correctly as well.

Comment by Stuart Halloway [ 22/Dec/10 9:03 PM ]

Second patch is good. This is more important because apparently the IDE REPLs use pprint by default.





[CLJ-697] Compiler doesn't push a binding for *unchecked-math* Created: 21/Dec/10  Updated: 01/Mar/13  Resolved: 05/Jan/11

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

Type: Defect Priority: Major
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: None

Attachments: File 697_unchecked_math.diff    
Patch: Code
Approval: Ok
Waiting On: Stuart Halloway

 Description   

Compile.java in Clojure doesn't push a binding for unchecked-math, so compilation of code with statements like (set! unchecked-math true) fails with:

Can't change/establish root binding of: unchecked-math with set

This problem doesn't happen with the REPL because with-bindings in clojure.main sets up an unchecked-math binding.






[CLJ-682] cl-format: ~w throws an exception when not wrapped in a pretty-writer Created: 27/Nov/10  Updated: 01/Mar/13  Resolved: 31/Dec/10

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

Type: Defect Priority: Minor
Reporter: Tom Faulhaber Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-682-patch.diff    
Patch: Code
Approval: Ok

 Description   

(cl-format nil "hello: w%" '(a b c))

throws an exception because ~w is (kind of ironically) not marked :pretty in the cl-format definition.



 Comments   
Comment by Tom Faulhaber [ 29/Dec/10 1:34 AM ]

This one-line patch resolves the issue.

Comment by Tom Faulhaber [ 29/Dec/10 1:35 AM ]

OK, this is ready to be applied to master.





[CLJ-689] Audit and update all user permissions on the hudson machine Created: 12/Dec/10  Updated: 01/Mar/13  Resolved: 22/Feb/11

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

Type: Task Priority: Critical
Reporter: Aaron Bedra Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

Currently, the small handful of users are all hudson administrators. With Stuart Sierra's pending changes to the build process, the hudson box will be doing all releases of Clojure. This means that only a few people should be able to run the -alpha, -beta, and stable releases.



 Comments   
Comment by Stuart Halloway [ 31/Dec/10 4:05 PM ]

For some reason this was incorrectly marked as OK. When the work is complete, please mark as Screened with a note showing Rich where to go to verify.

Comment by Aaron Bedra [ 03/Jan/11 7:31 AM ]

Bah! That shouldn't have happened. I got a 500 from JIRA after trying to update the ticket and figured I would visit it when I had more time. Looks like it ended up being set the wrong way. Anyways, the permissions have been updated and are in need of a review.

Comment by Stuart Halloway [ 05/Jan/11 6:50 AM ]

Please grab someone in the office and walk them through what you have done (documenting it here) so we can mark this complete.

Comment by Stuart Sierra [ 28/Jan/11 2:09 PM ]

Audit completed; results and recommendations communicated privately.

Comment by Stuart Sierra [ 28/Jan/11 3:52 PM ]

Verified Aaron's fixes.





[CLJ-149] pop does not preserve meta-data Created: 10/Jul/09  Updated: 01/Mar/13  Resolved: 19/Mar/11

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

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Aaron Bedra
Resolution: Declined Votes: 0
Labels: None

Patch: Code

 Description   

As discussed with chouser on irc.

Example:

=> (meta (conj (with-meta '(1 2 3) {:my :meta}) 4))
{:my :meta}
=> (meta (pop (with-meta '(1 2 3) {:my :meta}))) 
nil

Both calls should yield the same result.

This occurs in 1.0.0 as well as trunk.

This might be part of a larger issue with when meta-data should be preserved (e.g., butlast).



 Comments   
Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/149

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: Alex, sorry if my various statements mislead you. What you described may be a bug, but I don't know that for sure. Rich would have to rule on what the desired behavior is there.

However, I'm pretty sure that the metadata is getting lost here incorrectly:

<pre>
(let [v (with-meta [1 2 3] {:my :meta})]
(map meta [(conj v 4) (pop v)]))

returns: (nil nil)
</pre>

Because if you start with a PersistentVector instead of a LazilyPersistentVector, the metadata is preserved:

<pre>
(let [v (with-meta (pop [1 2 3]) {:my :meta})]
(map meta [(conj v 4) (pop v)]))

returns: ({:my :meta} {:my :meta})
</pre>

The problem seems to be that metadata is lost when the LazilyPersistentVector is converted to a PersistentVector. I'll attach a patch to address this.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: [file:dBRCkCBBar3QrKeJe5aVNr]: Move metadata from LazilyPersistentVectors to PersistentVectors when the latter are created.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

richhickey said: That diff doesn't seem to be the right one

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: Bother. I think I deleted my local copy of the correct patch after attaching the incorrect one here.

It just added .withMeta(meta()) in LazilyPersistentVector's v() method, plus some related unit tests. I'll recreate it when I get a chance.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Alexander Redington [ 12/Nov/10 10:15 AM ]

Reported behavior is still observed in the latest development sources.

Comment by Alexander Taggart [ 01/Mar/11 1:45 AM ]

PersistentList.pop() will now create a new head of the forthcoming list if it doesn't share the same metadata instance.

Comment by Stuart Halloway [ 04/Mar/11 3:27 PM ]

This works as advertised, but I am unclear on what should happen. Given

(a b)

where both the conses have metadata, what rule specifies which metadata should be left after pop? If there is a rule, we should document it, and I bet there are other breakages.

Comment by Rich Hickey [ 10/Mar/11 11:32 AM ]

This is a non-problem (but the vector one Chouser found is a real problem). Lists are not going to create new heads to convey metadata on pop. Lists really are chains of things, not encapsulations of chains of things. The metadata of the second link is what it was, and shouldn't be lost by pop, i.e. pop should not construct a new list as it goes.

Comment by Alexander Taggart [ 10/Mar/11 12:50 PM ]

Very well. Note that the failure example chouser gave no longer occurs:

user=> (let [v (with-meta [1 2 3] {:my :meta})]
         (map meta [(conj v 4) (pop v)]))
({:my :meta} {:my :meta})




[CLJ-191] enhance stacktrace: causes like java, syntax like clojure Created: 14/Sep/09  Updated: 01/Mar/13  Resolved: 31/Dec/10

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Various enhancements I'd like to make:
Copy the feature of java stacktrace to print "... 25 more" instead of duplicating stack lines.
Print java class.method in clojure format: class/method.

Discussion: http://groups.google.com/group/clojure-dev/browse_thread/thread/d03dd16acbe3aa14



 Comments   
Comment by Assembla Importer [ 01/Sep/10 9:39 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/191
Attachments:
clojure-191-stacktrace.patch - https://www.assembla.com/spaces/clojure/documents/dz8x6oOqar3PnLeJe5afGb/download/dz8x6oOqar3PnLeJe5afGb

Comment by Assembla Importer [ 01/Sep/10 9:39 AM ]

mikehinchey said: [file:dz8x6oOqar3PnLeJe5afGb]: enhance stacktrace

Comment by Assembla Importer [ 01/Sep/10 9:39 AM ]

mikehinchey said: Printing causes, abridge duplicated stack lines with ... (same as java printStackTrace does).
Improve matching and reformatting of stack lines of clj code.
(e) takes an optional argument. (e) prints all causes.
Created unit tests for stacktrace (succeeds in repl and ant).

Comment by Assembla Importer [ 01/Sep/10 9:39 AM ]

richhickey said: I'm not sure this is the patch, but I'd like to get improved stack traces on the agenda again. See also http://github.com/mmcgrana/clj-stacktrace and other ideas.

Comment by Assembla Importer [ 01/Sep/10 9:39 AM ]

richhickey said: Some work has been done in master, little feedback at present

Comment by Aaron Bedra [ 10/Dec/10 10:10 AM ]

Further discussion on this ticket is at http://dev.clojure.org/display/design/Stacktraces and should be the basis for any future discussion and eventual design sign off.

Comment by Stuart Halloway [ 31/Dec/10 3:55 PM ]

This will be subsumed under the changes being proposed at http://dev.clojure.org/display/design/Error+Handling





[CLJ-784] Update min/max functions to take advantage of primitive math Created: 29/Apr/11  Updated: 01/Mar/13  Resolved: 26/May/11

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Luke VanderHart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0784-min-max-take-3.patch     Text File fast_min_max_no_contagion.patch     Text File fast_min_max.patch    
Patch: Code
Approval: Ok

 Description   

Currently, the min and max functions are quite slow, since they don't take advantage of new primitive math.

Implement them in the same manner in which other primitive math functions are implemented for efficiency.



 Comments   
Comment by Luke VanderHart [ 29/Apr/11 3:17 PM ]

This patch creates inline versions of min and max, combined with overloaded implementations in c.l.Numbers. It is over an order of magnitude faster using a test like this one (from 19 seconds to 1.2 seconds on my machine):

(time (loop [a 0 b 10 c 5]
            (if (< a 1000000000)
                (recur (inc a) (min b c) (max c b)))))

There are two patches. One is contagious, but returns primitives for (long, double) input. The other is not contagious and returns primitives only for (long,long) and (double,double) args, but is still substantially faster than the original version.

Comment by Stuart Halloway [ 06/May/11 10:11 AM ]

The May 6 "take 3" patch eliminates a few contagions that were hiding in the earlier patch, and is hopefully good to go.





[CLJ-244] walk support for sorted-by collections Created: 13/Jan/10  Updated: 01/Mar/13  Resolved: 05/Jan/11

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Timothy Pratley
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0244-sorted-walk-resolved.patch    
Approval: Ok

 Description   

http://groups.google.com/group/clojure-dev/browse_thread/thread/51982e4091e3614d



 Comments   
Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/244
Attachments:
sorted_walk.diff - https://www.assembla.com/spaces/clojure/documents/b0Y9k2c5Or34IaeJe5afGb/download/b0Y9k2c5Or34IaeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

timothypratley said: [file:b0Y9k2c5Or34IaeJe5afGb]: fix and tests

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

timothypratley said: Ready

Comment by Stuart Halloway [ 04/Jan/11 8:18 PM ]

Jan 4 patch updated to work with current master.





[CLJ-716] Hudson release build failing at git clone step Created: 14/Jan/11  Updated: 01/Mar/13  Resolved: 14/Jan/11

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Approval: Ok
Waiting On: Stuart Halloway

 Description   

See http://build.clojure.org/job/clojure/237/console for specifics.



 Comments   
Comment by Stuart Sierra [ 14/Jan/11 2:26 PM ]

Looks like some of the advanced Git options were misconfigured in the Hudson job. Specifically:

  • "Checkout/merge to local branch" should be "master"
  • "Merge before build" should be checked
    • "Name of repository" should be "origin"
    • "Branch to merge to" should be "master"

I've changed these settings. Ready to try another release.





[CLJ-184] n-ary bit functions, also inlining of n-ary bit and math operations Created: 26/Aug/09  Updated: 01/Mar/13  Resolved: 14/May/11

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: None

Attachments: File 184-inlining-nary-math-2.diff     File 184-inlining-nary-math-3.diff     File 184-inlining-nary-math.diff     Text File inlinemath.patch    
Approval: Ok

 Description   

Add an &more version to the bit operations bit-and bit-or bit-xor bit-and-not (via reduce),
Inline the n-ary version of the math ops, + - * /, inline the n-ary versions of bit-and bit-or bit-xor bit-and-not



 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/184
Attachments:
inlinemath.patch - https://www.assembla.com/spaces/clojure/documents/dlFRFQKPyr3RaoeJe5aVNr/download/dlFRFQKPyr3RaoeJe5aVNr

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

Jonathan A Smith said: [file:dlFRFQKPyr3RaoeJe5aVNr]: inline math and bit-or patch file

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

Jonathan A Smith said: original math defs stay, new ones get re-defined after proxy is available.

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

mikehinchey said: Applies clean (though has extra whitespace and tabs). Existing tests still pass.

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

Jonathan A Smith said: The defn functions are within a let, which means that they get tabbed in by Emacs ctrl+alt+Q. Is this the referred to 'extra whitespace and tabs'?

I think that this line:
>0-arities (rule-set (fn [x] (> x 1)))

might be a mistake, and perhaps should be:
>0-arities (rule-set (fn [x] (> x 0)))

But I will have to look at it and rebuild it when I get home tonight. resulting tests would still pass as the only effected function would be the single arity subtraction operation.

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

mikehinchey said: "git am" complained about whitespace at the ends of lines. When I tab on each line to re-indent, emacs replaced tabs with spaces and deleted space at the end. Also, you could combine the 2 let blocks into 1.

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

richhickey said: This still needs work. I'd like to avoid the need for proxy. It would be easy to make Compiler.isInline() in the compiler presume :inline-arities is an IFn (so sets literals would still work, but so would predicate fns)

Comment by Assembla Importer [ 24/Aug/10 6:19 AM ]

richhickey said: I've made that change, so isInline now treats :inline-arities as a predicate IFn. This should allow for a simpler implementation

Comment by Christopher Redinger [ 22/Apr/11 11:40 AM ]

Adding patch from Assembla

Comment by Alan Dipert [ 30/Apr/11 3:59 PM ]

inlining and n-ary bit functions and math ops

  • n-ary versions and inlines of bit-and, bit-or, bit-xor, bit-and-not
  • n-ary inlines for +, +', *, *', /, -, -'

This patch doesn't optimize / such that the & more version expands to equivalent of (/ x (reduce * y more)) rather than (reduce / (/ x y) more). The original patch had a divide-by-zero bug, and I think adding this should be another ticket.

Comment by Alan Dipert [ 30/Apr/11 4:12 PM ]

Created CLJ-785 for optimizing n-ary /

Comment by Alexander Taggart [ 03/May/11 1:46 PM ]

You'd have less copypasta using this:

(defn ^:private binary-inline
  ([op]
    (fn
      ([x y] `(. clojure.lang.Numbers (~op ~x ~y)))
      ([x y & more]
        (reduce1
          (fn [a b] `(. clojure.lang.Numbers (~op ~a ~b)))
          `(. clojure.lang.Numbers (~op ~x ~y)) more))))
  ([op unchecked-op]
    (if *unchecked-math*
      (binary-inline unchecked-op)
      (binary-inline op))))

(defn ^:private >1? [n] (clojure.lang.Numbers/gt n 1))

Also, ^:static doesn't do anything anymore.

The above was from the branch I made, that I mentioned in the thread on ticket status. It also included a fix for the inefficient divide.

Comment by Alan Dipert [ 03/May/11 8:50 PM ]

Thanks, I'll extract a patch for this from your branch.

Comment by Alan Dipert [ 06/May/11 12:39 PM ]

This patch incorporates Alex Taggart's work in the comments, but leaves out the change to /. It made a call to the nonexistent Numbers/unchecked_divide method and multiplies in the denominator, which is faster but has different overflow behavior than divide/reduce.

Comment by Alexander Taggart [ 07/May/11 12:30 AM ]

One problem with the patch: while the bit-ops fns have n-ary support for their inlines, the fns themselves do not. They need to be updated to something like:

(defn bit-and
  "Bitwise and"
  {:inline (nary-inline 'and)
   :inline-arities >1?
   :added "1.0"}
  ([x y] (. clojure.lang.Numbers and x y))
  ([x y & more]
    (reduce1 bit-and (bit-and x y) more)))
Comment by Alan Dipert [ 09/May/11 11:42 PM ]

Attached 184-inlining-nary-math-3.diff. Same as previous patch, but adds n-ary version of bit fns in addition to inlines, which were missing as Alex Taggart pointed out. Thanks for catching that!

Comment by Alan Dipert [ 14/May/11 4:21 PM ]

Patch is in 1.3-alpha7





[CLJ-234] Adding provenance of deprecation warnings to the LispReader Created: 01/Jan/10  Updated: 01/Mar/13  Resolved: 31/Dec/10

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

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

Attachments: Text File 0234_LispReader_deprecation_provenance.patch     Text File 0234_LispReader_deprecation_provenance.patch    
Approval: Vetted

 Description   

The LispReader has some deprecation warnings, but they do not show their provenance. The attached patch provide improved warning messages by modifying the LineNumberingPushbackReader to include the source path of the file being read, so that it can be retrieved by the LispReader.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 7:02 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/234
Attachments:
0002-Modified-the-LineNumberingPushbackReader-to-include-.patch - https://www.assembla.com/spaces/clojure/documents/bmHcpC9Yur3RGweJe5afGb/download/bmHcpC9Yur3RGweJe5afGb

Comment by Stuart Halloway [ 17/Dec/10 2:31 PM ]

Alan: please bounce this if incomplete, or screen if it seems simple enough.

Comment by Alan Dipert [ 17/Dec/10 2:53 PM ]

The patch looks good, but as of 1.2 there are no longer any deprecated reader macros. This might be worth adding in case any macros are deprecated in the future.

Comment by Stuart Halloway [ 31/Dec/10 3:59 PM ]

If we ever have another deprecation, we should bring this back along with a test case.





[CLJ-736] Docstring of defrecord (and =) does not correctly describe equality behavior for records. Created: 09/Feb/11  Updated: 01/Mar/13  Resolved: 02/Sep/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4
Fix Version/s: Release 1.3, Release 1.4

Type: Enhancement Priority: Minor
Reporter: Jason Wolfe Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Approval: Accepted
Waiting On: Stuart Halloway

 Description   

http://groups.google.com/group/clojure/browse_frm/thread/eba691b38c45196b#

The docstring of defrecord says it "will define type-and-
value-based equality and hashCode". In reality, record types are included in = but not .equals (or hashCode), and so records of different types can collide as map keys.

Along the same lines, the docstring of = says "same as Java x.equals, except it ... compares numbers and collections in a type-independent manner". To me, this seems to imply the opposite of what actually happens with records.

FWIW I think it would be more clear if the behavior for = and .equals were the same in this respect, but in any case the implemented behavior should be properly documented.



 Comments   
Comment by Stuart Sierra [ 06/Jun/11 9:37 AM ]

Also discussed at https://groups.google.com/d/topic/clojure/e6UhXVny8Xc/discussion

Per Rich, "The policy is: = includes the type and .equals doesn't. In this way records can still be proper j.u.Maps and = remains most useful for records (e.g. including type)."

Comment by Jason Wolfe [ 06/Jun/11 11:25 AM ]

On a related note:

http://dev.clojure.org/display/doc/Enhanced+Primitive+Support?focusedCommentId=1573146#comment-1573146

Regarding "hash maps and sets now use = for keys ... will use stricter .equals when calling through java.util interfaces":

Note that "=" is actually stricter than ".equals" for record types currently, because it includes type information.

This has important consequences for how (and whether) hash maps and sets actually obey the java.util interfaces.

For instance, if

(defrecord P []), ...

what should (.get {(P.) 1 (Q.) 2} (Q.)) return? How about if we .get an element of a third type (R.)?

In a similar vein, this behavior seems confusing at best:

user=> (java.util.HashMap. {(P.) 1 (Q.) 2})
#<HashMap {user.P@0=2}>

Comment by Aaron Bedra [ 28/Jun/11 6:36 PM ]

Open question:

  • the phrase "type-independent" is confusing. It is intended to talk about concrete numeric and collection types, but I can see why it gets confusing with records. Need an idea for two different terms here (and define them someplace). If somebody has one, please add it to this ticket.

Non-issues:

  • = and .equals are not and will not be the same. The whole point is to avoid Java's broken semantics (=) while providing an interop formula for those who need it (.equals)
  • Any types can collide as hash keys.
Comment by Aaron Bedra [ 19/Aug/11 11:34 AM ]

Any thoughts on the open questions part Rich?

Comment by Stuart Halloway [ 02/Sep/11 9:37 AM ]

I think the following is better:

monospaced
In addition, defrecord will define type-and-value-based =,
and will defined Java .hashCode and .equals consistent with the
contract for java.util.Map.
monospaced

But I don't want to hold release for discussion, so bumping this to approved backlog for further bikeshedding.

Comment by Stuart Halloway [ 02/Sep/11 9:39 AM ]

Better yet, let's just agreed on my prose and call this done.





[CLJ-681] Build Clojure with Maven 2 Created: 26/Nov/10  Updated: 01/Mar/13  Resolved: 05/Jan/11

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0681-patch-10.patch     Text File 0681-patch-11.patch     GZip Archive maven-build-1.diff.gz     GZip Archive maven-build-2.diff.gz     GZip Archive maven-build-3.diff.gz     GZip Archive maven-build-4.diff.gz     GZip Archive maven-build-5.diff.gz     GZip Archive maven-build-6.diff.gz     GZip Archive maven-build-7.diff.gz     GZip Archive maven-build-8.diff.gz     GZip Archive maven-build-9.diff.gz    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

See Common Contrib Build wiki page and the lengthy clojure-dev discussion

For examples of the CI/release process under Hudson, see clojure-testbuild



 Comments   
Comment by Stuart Sierra [ 26/Nov/10 2:09 PM ]

Replaces previous patch.

  • fixed SCM URLs in pom.xml
Comment by Stuart Halloway [ 29/Nov/10 6:48 AM ]

Maven is capable of using the directory structure we already have. It seems this patch is doing two things: getting us onto maven, and reorganizing the project. Is there any reason these can't be considered separately?

Comment by Stuart Sierra [ 29/Nov/10 6:28 PM ]

Updated patch replaces previous patches.

This does minimal file/directory renaming while preserving the same functionality as the previous patches.

But an older problem has resurfaced: the nondeterministic load-order sometimes causes compilation to fail. For example, on Hudson

Comment by Stuart Sierra [ 01/Dec/10 8:05 AM ]

Patch maven-build-4.diff.gz replaces previous patches.

This works around compilation-order issues by using a bootstrap script to compile namespaces in the correct order.

Comment by Stuart Sierra [ 01/Dec/10 8:59 AM ]

Patch maven-build-5.diff.gz replaces previous patches.

This fixes a few more testing issues.

Comment by Stuart Sierra [ 01/Dec/10 9:02 AM ]

Finally got a successful staging release

Unfortunately, the JAR file is misnamed. This is a clojure-maven-plugin bug that will be fixed in the next release.

Comment by Stuart Sierra [ 03/Dec/10 9:39 AM ]

Patch maven-build-6.diff.gz replaces previous patches.

This updates to clojure-maven-plugin version 1.3.7; README updates.

Comment by Stuart Sierra [ 03/Dec/10 4:13 PM ]

Patch maven-build-7.diff.gz replaces previous patches.

This patch adds back in a simplified build.xml for local development using Ant only.

The Ant build.xml script reads the project version number from pom.xml, so pom.xml remains the definitive project description.

Comment by Stuart Sierra [ 09/Dec/10 5:14 PM ]

Patch maven-build-8.diff.gz replaces previous patches.

This version uses a hybrid Maven + Ant build process. For local development, only Ant is needed. The POM does not depend on anything outside of the standard Maven toolchain and the Sonatype open-source deployment configuration.

Comment by Stuart Sierra [ 15/Dec/10 12:01 PM ]

Patch "maven-build-8" answers those questions by postponing them.

This patch is ready to test and apply.

Comment by Stuart Sierra [ 15/Dec/10 12:04 PM ]

To elaborate: patch "maven-build-8" avoids both clojure-maven-plugin and bootstrap load order by using maven-ant-plugin and Ant to drive the Clojure steps in the build.

We still need to address these issues, but that may take a long time, and we want to move forward with releases to public repositories.

Comment by Stuart Halloway [ 17/Dec/10 2:05 PM ]

Can you please run down two small issues:

(1) I get a warning on the maven build:

[WARNING] 
[WARNING] Some problems were encountered while building the effective model for org.clojure:clojure:jar:1.3.0-testbuild10-SNAPSHOT
[WARNING] 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-compiler-plugin is missing. @ line 52, column 15
[WARNING] 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-surefire-plugin is missing. @ line 181, column 15
[WARNING] 
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING] 
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING]

(2) The version is set to 1.3.0-testbuild10-SNAPSHOT. Am I supposed to change this by hand-editing the pom?

Comment by Stuart Sierra [ 17/Dec/10 4:10 PM ]

Patch maven-build-9.diff.gz replaces previous patches.

Differences from previous patches:

  • Adds plugin coordinates to fix the warnings reported by Stu Halloway
  • Avoids compiling the test sources twice
  • Correctly sets POM version to 1.3.0-master-SNAPSHOT
Comment by Stuart Sierra [ 17/Dec/10 4:11 PM ]

Reported issues should be fixed, although I cannot reproduce the warning messages.

What version of Maven produced those warnings?

Comment by Stuart Halloway [ 04/Jan/11 4:03 PM ]

Patch 10 merely updates patch 9 to apply to current master. (A test namespace had changed. The list of test files has always been hard-coded and fragile, but that's a patch for another day.)

I tested all the Ant and Maven commands documented in the reademe, and everything looks sensible.

Comment by Stuart Halloway [ 04/Jan/11 9:03 PM ]

Patch 10 is bad. The pom file from patch 9 was lost, and my local environment didn't notice somehow. Will test and resubmit a patch 11 that is basically patch 10 with the correct pom.xml from patch 9.

Grumble.

Comment by Stuart Halloway [ 05/Jan/11 7:18 AM ]

Patch 11 is basically patch 10 + the correct pom.xml from patch 9.





[CLJ-780] race condition in reference cache on Java 5 Created: 27/Apr/11  Updated: 01/Mar/13  Resolved: 27/Apr/11

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File check-map-entry-value.patch    
Patch: Code
Approval: Ok

 Description   

Map.Entry instances can have null values prior to Java 6

Test failure: http://build.clojure.org/job/test.generative/1/org.clojure$test.generative/console

Discussion: http://concurrency.markmail.org/message/pbruo2uxgur6wkoo?q=map%2Eentry+null&page=3#query:map.entry%20null+page:3+mid:z5arnbbzl2k32jda+state:results

Note that the fix must hold val in a local variable, because cache.remove will bomb if the value is null.






[CLJ-752] Remove *earmuff* == dynamic support Created: 09/Mar/11  Updated: 01/Mar/13  Resolved: 08/Apr/11

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

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

Attachments: Text File 752.patch    
Patch: Code
Approval: Ok

 Description   

As a compatibility bridge, when dynamic var support was first added, *earmuffed* vars were automagically considered dynamic. Users were promised this bridge would be removed before release. Let's get this change in so people can start dealing with it.



 Comments   
Comment by Alexander Taggart [ 09/Mar/11 1:59 PM ]

Remove the warning message as well?

Comment by Rich Hickey [ 09/Mar/11 2:05 PM ]

The warning message should be changed to say:

Warning: *x* not declared dynamic and thus is not dynamically rebindable, but its name suggests otherwise. Please either indicate ^:dynamic *x* or change the name.

Comment by Alexander Taggart [ 09/Mar/11 2:34 PM ]

752.patch removes inferring ^:dynamic from earmuffed var; updates warning message.





[CLJ-816] Transient vectors mutations leak into their source persistent vector Created: 24/Jun/11  Updated: 01/Mar/13  Resolved: 19/Aug/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.1, Release 1.2, Release 1.3
Fix Version/s: Release 1.3

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Completed Votes: 0
Labels: None

Attachments: File fix-transientvectors.diff    
Patch: Code and Test
Approval: Ok

 Description   

(let [pv (vec (range 34))]
(-> pv transient pop! pop! pop! (conj! 42))
(nth pv 31))
; returns 42 instead of 31



 Comments   
Comment by Christophe Grand [ 24/Jun/11 11:50 AM ]

The patch may apply to 1.1 and 1.2 too.





[CLJ-751] cl-format: ~( thows an exception with an empty string Created: 05/Mar/11  Updated: 01/Mar/13  Resolved: 08/Apr/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3
Fix Version/s: Release 1.3

Type: Defect Priority: Minor
Reporter: Tom Faulhaber Assignee: Tom Faulhaber
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-751.diff    
Patch: Code and Test
Approval: Ok

 Comments   
Comment by Tom Faulhaber [ 05/Mar/11 7:44 PM ]

The following block of code throws an index out of range exception:

(cl-format nil "~:(~a~)" "")
Comment by Tom Faulhaber [ 06/Mar/11 12:01 AM ]

Patch that fixes the issue and adds tests

Comment by Stuart Halloway [ 20/Mar/11 10:17 AM ]

Patch works. One question: Why does cl-format print "nil" as "Nil"? Is this a CL-ism? Do we want it?

Comment by Tom Faulhaber [ 21/Mar/11 10:51 AM ]

Answer: cl-format doesn't print nil as "Nil". The () construction is a case control operator. In this case, it capitalizes the each word in the expression: http://www.lispworks.com/documentation/HyperSpec/Body/22_cha.htm.

In general, I've suppressed the obvious "CL-only" stuff like upper case symbols and such and replaced them with the corresponding Clojure conventions.





[CLJ-433] munge should not munge $ (which isJavaIdentifierPart), should munge ' (which is not) Created: 09/Sep/10  Updated: 01/Mar/13  Resolved: 05/Jan/11

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

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-fix-munge-handling-of.patch    
Approval: Ok

 Description   

munge is overeager in converting $ to DOLLARSIGN, since $ is a valid
character in Java identifiers:

user> (filter #(Character/isJavaIdentifierPart %) (keys
clojure.lang.Compiler/CHAR_MAP))
(\$)

On a related point, ' (single quote) is not admissible in Java
identifiers, but it is not munged on master:

user=> (Character/isJavaIdentifierPart \')
false
user=> (munge "'")
"'"

This leads to e.g.

user=> *'
#<core$_STAR_' clojure.core$_STAR_'@5adf48c4>

(note the ' after STAR).

Originally reported on the Dev list.

See also this thread on the (regular) Clojure ggroup.

The attached patch applies cleanly against current master.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 4:32 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/433
Attachments:
0001-munge-no-longer-changes-to-DOLLARSIGN.patch - https://www.assembla.com/spaces/clojure/documents/dp5tziVaer34yIeJe5cbLr/download/dp5tziVaer34yIeJe5cbLr

Comment by Stuart Halloway [ 30/Oct/10 12:12 PM ]

Patch is clean, the real issue here is doing the right thing. Two concerns:

  • was the original choice to munge $ motivated, and if so do we need a more subtle patch that preserves that original intent?
  • presumably this is (yet another) binary-compatibility-breaking change. 1.3 already has major change, so now is a good a time as any...
Comment by Rich Hickey [ 05/Nov/10 8:00 AM ]

This issue mentions two problems but patch fixes only one. Should add the single-quote handling

Comment by Michał Marczyk [ 08/Dec/10 12:12 AM ]

Right, I forgot about that in the patch somehow. Also, I just noticed that " is also not JavaIdentifierPart and yet is not currently munged. The newly attached patch fixes all three issues.





[CLJ-817] print-deftype breaks when fields have dashes Created: 27/Jun/11  Updated: 01/Mar/13  Resolved: 22/Mar/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.3, Release 1.4

Type: Defect Priority: Minor
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: None

Attachments: File 817-print-deftype-fields-dashes.diff     Text File test.log    

 Description   
user=> (deftype Foo [bar-quux])
user.Foo
user=> (def x (Foo. 1))
#'user/x 
user=> x
IllegalArgumentException No matching field found: bar-quux for class user.Foo  clojure.lang.Reflector.getInstanceField (Reflector.java:289)
#user.Foo[user=>

Printing records works fine. See this mailing list message for original report.



 Comments   
Comment by Aaron Bedra [ 28/Jun/11 12:59 PM ]

This patch breaks tests.

Comment by Alan Dipert [ 28/Jun/11 2:18 PM ]

Could you please attach your test output? I'm not able to reproduce.

Comment by Aaron Bedra [ 29/Jun/11 7:37 AM ]

I attached the full test run. This is

java version "1.6.0_24"
Java(TM) SE Runtime Environment (build 1.6.0_24-b07)
Java HotSpot(TM) 64-Bit Server VM (build 19.1-b02, mixed mode)

on Ubuntu 11.04 64

Comment by Alan Dipert [ 06/Jul/11 10:39 PM ]

I haven't been able to reproduce your error. Could you try ant clean and run again? Perhaps another patch was also applied when you tested? Thanks in advance for giving it another shot.

Platforms I've tried that compile and test cleanly:

  • Mac OS X 10.6.8, 1.6.0_24-b07
  • Ubuntu 11.04 32, Sun JDK 1.6.0_24-b07
  • Ubuntu 11.04 64, Sun JDK 1.6.0_24-b07 (EC2, ami-1aad5273)
  • Ubuntu 11.04 64, OpenJDK 1.6.0_22 (EC2, ami-1aad5273)
  • CentOS 5.6 64, Sun JDK 1.6.0_27-ea
Comment by Kevin Downey [ 29/Nov/11 7:42 PM ]

this patch no longer applies cleanly

Comment by Andy Fingerhut [ 24/Feb/12 4:50 PM ]

The bad behavior appears not to exist in latest master as of Feb 24, 2012, nor does it exist in release 1.3.0. The code in the neighborhood of this one-line patch is significantly different than it was when the patch was made, so perhaps this issue was corrected as a side effect of some commit before the 1.3.0 release.

Mac OS X 10.6.8, 1.6.0_29, Clojure 1.3.0 test transcript:

Clojure 1.3.0
user=> (deftype Foo [bar-quux])
user.Foo
user=> (def x (Foo. 1))
#'user/x
user=> x
#<Foo user.Foo@6d080876>
user=> (deftype Bar [bar-id])
user.Bar
user=> (Bar. 1)
#<Bar user.Bar@679801c>
user=> (deftype Baz [bar_id])
user.Baz
user=> (Baz. 1)
#<Baz user.Baz@54128635>

Comment by Alan Dipert [ 22/Mar/12 8:21 PM ]

This issue does not exist in Clojure 1.3 onwards, and the attached patch is no longer relevant.





[CLJ-796] Records with fields named "values" Created: 16/May/11  Updated: 01/Mar/13  Resolved: 18/May/11

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

Type: Defect Priority: Minor
Reporter: David McNeil Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

It looks to me like there is a problem in records when the field name "values" is used:

(defrecord Foo [values])

(= (Foo. 1)
(Foo. 1))

;; -> false



 Comments   
Comment by Fogus [ 18/May/11 7:37 AM ]

Hi David,

This condition seems to go back to 1.2 and still exists in version 1.3.0-alpha7. We'll take a look.

Thank you

Comment by Fogus [ 18/May/11 7:45 AM ]

The problem seems to occur whenever a field name conflicts with a name of one of the record's protocol functions:

(defrecord R [isEmpty])

(= (R. 42) (R. 42))
;=> false

(defrecord RR [count])

(= (RR. 42) (RR. 42))
;=> false

Either this should be made to work, or an error signalled regarding a name clash. Anyone have an opinion?





[CLJ-759] 1.3 alpha 6 seems to hold on to head in a case when 1.2 does not Created: 15/Mar/11  Updated: 14/Feb/12  Resolved: 14/Feb/12

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

I tested on both Mac OS X 10.6.6 with Hotspot 64-bit JVM 1.6.0_24, and Ubuntu Linux 10.4 LTS with Hotspot 64-bit JVM 1.6.0_24. I suspect the issue is similar across JVMs.


Attachments: File fasta.clj-11.clj     File make-input-file.sh     File read.clj     File run.sh    

 Description   

File names here refer to attached files. Instead of attaching a large input file, I attach a small program that can be run to generate a large input file. To generate it, edit make-input-file.sh to point to a Clojure 1.2 JAR. Also edit run.sh to point to a Clojure 1.2 and 1.3 alpha 6 JAR. Do the following one time to create a ~ 125 Mbyte file long-input.txt:

./make-input-file.sh

With the JVMs I tested, I got these results, where the first argument is 1.2 or 1.3 for the Clojure version, and the second argument is the number of Mbytes to use for the max heap size of the JVM (i.e. the numeric value in the -Xmx<num>m argument in the java command line):

./run.sh 1.2 384 # failed with OutOfMemoryError 5/5 times tried
./run.sh 1.2 416 # succeeded 5/5 times tried

./run.sh 1.3 960 # failed 5/5 times tried
./run.sh 1.3 1048 # succeeded 5/5 or 4/5 times tried, for Mac OS X or Linux, respectively

I believe the issue is that with 1.3 alpha 6, the JVM is holding on to the head of the sequence called 'lines' for the duration of the execution of the function fasta-dna-str-with-desc-beginning, whereas 1.2 is losing the head after getting past the first when-let, and thus using significantly less memory.



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 8:18 AM ]

Works fine with 416MB using what will become clojure-1.4.0 (commit 59d3c724684c212fbb5eafaaaac30761c2c75a37).

Comment by Andy Fingerhut [ 14/Feb/12 1:05 AM ]

Thanks for checking, Tassilo. I also verified that the head-holding issue is gone in 1.3.0 and latest 1.4.0-beta1 as of Feb 13, 2012 (commit e27a27d9a6dc3fd0d15f26a5076e2876007e0ae6). I will mark this issue resolved, although I don't know which particular changes did it.





[CLJ-836] BigInt optimization breaks "obvious" math Created: 06/Sep/11  Updated: 23/Sep/11  Resolved: 23/Sep/11

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

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0836.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

BigInt optimization seems seriously broken:

user=> (def a 1N)
#'user/a
user=> (* (+ a 10000000000000000) (+ a 10000000000000000))
ArithmeticException integer overflow
clojure.lang.Numbers.throwIntOverflow (Numbers.java:1374)

A BigInt is optimized back to a long and then overflows which is not
what happened in Beta1 and earlier.



 Comments   
Comment by Stuart Halloway [ 06/Sep/11 6:10 PM ]

Please see https://github.com/clojure/test.generative/commit/9a23bc4c8a713c690e5ac7d5377f7bad861e7489 for a partial regression test. I am going to continue to expand the test but wanted to get this patch in asap.





[CLJ-673] RT.load doesn't work when called by code that is on the bootstrap classpath Created: 08/Nov/10  Updated: 23/Sep/11  Resolved: 23/Sep/11

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

Type: Defect Priority: Minor
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: File fix-clj-637.diff    
Patch: Code
Approval: Screened

 Description   

RT.baseLoader assumes Compiler.getClass.getClassLoader will return a sane value. If Clojure is placed on the bootclasspath, it will return null.

Relying on baseLoader less may be possible; it looks like the ClassLoader/getSystemResource static method may get us the access we need to the bootstrap classloader, which would solve the problem I first noticed with using RT.load with the bootstrap classloader. However, there may be other issues beyond this.

More details at http://groups.google.com/group/clojure/browse_thread/thread/16c694573bd29552

I plan on investigating this further, but I've created this ticket to explain what I've discovered so far for posterity, etc.

Note that placing Clojure on the bootclasspath cuts its startup time in half; it's a measure that Charles Nutter of the JRuby project recommends.



 Comments   
Comment by Kevin Downey [ 29/Jun/11 10:04 PM ]

'lo as if from nowhere a patch!

Comment by Aaron Bedra [ 06/Aug/11 10:17 AM ]

Patch looks reasonable, but I am waiting for a response from Phil to see if this resolves his issue.

Comment by Phil Hagelberg [ 11/Aug/11 7:56 PM ]

I just tested it again with the latest Clojure from git and it works fine. Here's an easy test that breaks without the fix: java -Xbootclasspath/a:clojure-1.3.0-master-SNAPSHOT.jar clojure.main -e "(require 'clojure.pprint)"





[CLJ-815] [Documentation] - Remove unnecessary "Alpha" labels in docstrings Created: 24/Jun/11  Updated: 02/Sep/11  Resolved: 02/Sep/11

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

Type: Defect Priority: Minor
Reporter: Aaron Bedra Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Approval: Accepted

 Description   

From smfadi@googlemail.com on the Clojure mailing list

Just a quick question (before I forget): Is deftype/defrecord still
alpha? Or is the "Alpha - subject to change" note in the docstrings
just an oversight?

This needs some housekeeping. Here are a list of functions that are currently marked as "Alpha"

  • clojure.pprint/print-table
  • clojure.core/add-watch
  • clojure.core/remove-watch
  • clojure.core/juxt
  • clojure.core/transient
  • clojure.core/persistent!
  • clojure.core/conj!
  • clojure.core/assoc!
  • clojure.core/dissoc!
  • clojure.core/pop!
  • clojure.core/disj!
  • clojure.core/promise
  • clojure.core/deliver
  • clojure.core/defrecord (inside core_deftype)
  • clojure.core/deftype (inside core_deftype)
  • clojure.reflect/type-reflect
  • clojure.reflect/reflect


 Comments   
Comment by Aaron Bedra [ 24/Jun/11 10:56 AM ]

Rich, can we talk about which of these should be updated and which should not?

Comment by Aaron Bedra [ 19/Aug/11 11:37 AM ]

Bump

Comment by Stuart Halloway [ 02/Sep/11 9:41 AM ]

just juxt, for now





[CLJ-811] Support hinting arg vectors Created: 17/Jun/11  Updated: 26/Aug/11  Resolved: 26/Aug/11

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: Java Source File 811.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

Currently, return type hints must be on function or var names, e.g.:

(defn ^String foo [])

This is in contrast to primitive return type declarations, which must be on function arg vectors:

(defn foo ^long [])

The latter is preferred because:

  • Different arities of the same function can be typed differently
  • All of the type information for a given function hangs off of a single value (the arg vector)

For these reasons, supporting type hints on arg vectors as well is desirable. This would remove the (confusing and purely historic at this point) syntactic difference between type hints and signature declarations (with the current function/var name hinting support to be potentially deprecated and removed in the future).

As a pleasant side effect, this would disambiguate the semantics of var :tag metadata, thereby resolving CLJ-140.



 Comments   
Comment by Chas Emerick [ 29/Jun/11 2:30 AM ]

Patch attached (811.diff); change viewable on github here.

This makes calls prefer hints on arg vectors, but preserves fallback to hints on vars, so backwards compatibility is retained. The way is open to only ever use var tags when referring to their contents (rather than using var tags for calls of var contents)...presumably a deprecation/change to be scheduled later that would resolve CLJ-140.

Functions with a vararg signature are explicitly supported. Protocol functions' vars fell right into place.

Comment by Aaron Bedra [ 23/Aug/11 7:13 PM ]

Reviewed code and tested with argos against all contrib libraries that work on 1.3 with no hiccups including core.logic





[CLJ-824] BigInt math is slow when values of a BigInt are small enough to actually be treated as Longs Created: 06/Aug/11  Updated: 26/Aug/11  Resolved: 26/Aug/11

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

Type: Defect Priority: Major
Reporter: Aaron Bedra Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0824_faster_bigint_ops.patch     Text File 0824-sans-inc-dec.patch     Text File bigint.patch    
Patch: Code
Approval: Screened
Waiting On: Rich Hickey

 Description   

When doing math ops on a BigInt, It should default to treating smaller values of BigInts as Longs when possible to improve performance, and fall through to the actual BigInteger math when necessary.



 Comments   
Comment by Aaron Bedra [ 06/Aug/11 9:34 AM ]

This is a first attempt at this. I noticed some nice performance improvements. My question is around what and how much to test this. There are no tests in this patch but I would be happy to add some if they are necessary.

Comment by Aaron Bedra [ 12/Aug/11 1:15 PM ]

Ok, new patch. This one now passes all of the math tests in the test.generative examples

Comment by Stuart Halloway [ 23/Aug/11 6:12 PM ]

0824-sans-inc-dec is good. Same as Aaron's second patch, but removes BigInt inc and dec. They were left over from code that had them wired to Numbers.java, where they weren't any faster.





[CLJ-777] Release notes for 1.3 Created: 22/Apr/11  Updated: 23/Aug/11  Resolved: 23/Aug/11

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

Type: Enhancement Priority: Minor
Reporter: Christopher Redinger Assignee: Christopher Redinger
Resolution: Completed Votes: 0
Labels: None

Attachments: File 1.3-beta-release-notes-4.diff     File 1.3-beta-release-notes.diff     File 1.3-beta-release-notes.diff     File 1.3-beta-release-notes.diff    
Approval: Ok

 Description   

Go through issues and commit logs and assemble Release Notes

http://dev.clojure.org/display/doc/1.3



 Comments   
Comment by Christopher Redinger [ 13/May/11 2:39 PM ]

With the alpha7 stuff included

Comment by Stuart Sierra [ 31/May/11 8:39 AM ]

Patch does not apply on master as of commit 66a88de9408e93cf2b0d73382e662624a54c6c86.

Comment by Stuart Sierra [ 31/May/11 9:06 AM ]

Text error: under section "1.4 Removed Bit Operation support for boxed numbers" there is a second, unrelated sentence that reads "Replicate has been deprecated in favor of repeat"

Comment by Stuart Sierra [ 01/Jun/11 3:16 PM ]

New patch "1.3-beta-release-notes-4.diff" subsumes Redinger's patch of 31/May/11 and adds a few fixes to the text of changes.txt.





[CLJ-812] print-dup should not be defined for deftypes Created: 20/Jun/11  Updated: 29/Jul/11  Resolved: 29/Jul/11

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

Type: Defect Priority: Major
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJ-812-print-dup-no-deftype.diff     File CLJ-812-print-dup-no-deftype.diff    
Patch: Code and Test
Approval: Ok

 Description   

deftypes are supposed to be relatively 'clean'. Defining print-dup on them might conflict with their other intended uses - e.g. if you use a deftype and implement IPersistentSet you can't print due to this conflict.



 Comments   
Comment by Fogus [ 20/Jun/11 11:55 AM ]

Added patch to back out the deftype print-dup.

Comment by Rich Hickey [ 20/Jun/11 12:12 PM ]

Erm, ditto print-method.

Also, was this put in originally in order to support deftypes in code?

Comment by Fogus [ 20/Jun/11 1:12 PM ]

Ditto print-method.

Replaces the previous patch.

Comment by Fogus [ 20/Jun/11 1:16 PM ]

Rich,

Removed print-method also.

print-dup was initially put in lieu of compiler support, its remaining was not intended. print-method was put to provide a prettier representation and I was not aware of the larger implications. Thank you for the ticket, I hope they didn't cause any annoyances.





[CLJ-798] BigInteger is print-dup'able but not readable Created: 16/May/11  Updated: 29/Jul/11  Resolved: 29/Jul/11

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

Type: Defect Priority: Minor
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File print-dup-big-integer.patch     Text File readable-BIGINT.patch     Text File readable-BIGINT-update-1.patch     Text File remove-biginteger-print.patch    
Patch: Code and Test
Approval: Ok

 Description   

Per Rich, "We should never be printing something that can't be read."

user=> (binding [*print-dup* true] (print-str (java.math.BigInteger. "1")))
"1BIGINT"
user=> (read-string *1)
NumberFormatException Invalid number: 1BIGINT  clojure.lang.LispReader.readNumber (LispReader.java:253)

This can either be resolved by removing the print-dup for BigInteger, printing it using the N notation (thus being read in as a BigInt), or adding read support for the BIGINT notation.



 Comments   
Comment by Alexander Taggart [ 21/May/11 9:58 PM ]

Patch emits BigIntegers using the N notation.

Apply patch after CLJ-799.

Comment by Christopher Redinger [ 25/May/11 2:20 PM ]

See discussion on clojure-dev mailing list: https://groups.google.com/d/topic/clojure-dev/VHvIGmS3HGw/discussion

Comment by Christopher Redinger [ 27/May/11 11:02 AM ]

Commentary on the Rich quote: We should read in the same thing that we wrote out. I think the preferred solution would be to read BigIntegers back in as BigIntegers.

Comment by Alexander Taggart [ 27/May/11 1:25 PM ]

readable-BIGINT.patch: makes the BIGINT notation readable to a java.lang.BigInteger.

Comment by Christopher Redinger [ 27/May/11 3:06 PM ]

Tests don't apply cleanly on latests master (due to us taking in CLJ-797 in alpha 8 without this one).

Would you mind refreshing this patch?

Comment by Alexander Taggart [ 30/May/11 1:26 PM ]

readable-BIGINT-update-1.patch: applies to master

Comment by Stuart Sierra [ 31/May/11 8:33 AM ]

Patch readable-BIGINT-update-1.patch applies on master as of commit 66a88de9408e93cf2b0d73382e662624a54c6c86.

However, I find this notation confusing. "BIGINT" looks like BigInt but actually means BigInteger.

If Clojure uses BigInt everywhere instead of BigInteger, I recommend removing this literal syntax.

Comment by Rich Hickey [ 07/Jun/11 8:45 PM ]

Let's please remove print-dup for BigIntegers. None of these other ideas have been approved. I don't want new literal syntax, nor print-dup taking one thing and reading as another. I especially don't want to have to discover what the plan is by reading the patch, The plan should be approved up front and be clear from the description.

Comment by Alexander Taggart [ 09/Jun/11 1:21 AM ]

remove-biginteger-print.patch: removes both print-dup and print-method methods for BigInteger. Now defaults to Number's methods.

And I strongly agree that patches should come after "the plan", but in the face of silence from the planners, some of us might feel inclined to make ready with some potential solutions while we have some time to spare.

Comment by Christopher Redinger [ 21/Jun/11 6:35 PM ]

Rich: I am interpreting the "no new literal syntax" part of your comment to mean that you don't want the "BIGINT" which is in print-method, therefore print-method is out, as well as print-dup. If that is the correct interpretation, this patch looks right.





[CLJ-808] In java.io, the make-parents function can't handle files without parent directory. Created: 08/Jun/11  Updated: 29/Jul/11  Resolved: 29/Jul/11

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

Type: Defect Priority: Trivial
Reporter: Nicolas Buduroi Assignee: Nicolas Buduroi
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fixes-java.io-make-parents-to-not-throw-a-NullPointe.patch     Text File 0808-idiomatic.patch    
Patch: Code
Approval: Ok

 Description   

The make-parents function throws a null pointer exception when given a simple local directory file.

user> (io/make-parents "test")

Thrown class java.lang.NullPointerException



 Comments   
Comment by Christopher Redinger [ 21/Jun/11 6:46 PM ]

Second patch is more idiomatic.





[CLJ-755] automate distribution zip Created: 11/Mar/11  Updated: 29/Jul/11  Resolved: 29/Jul/11

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

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

Approval: Ok
Waiting On: Stuart Halloway

 Description   

In the move to maven, we lost the old ant automation for creating a release zip. In order to release 1.3.0-alpha6, I am using the following ant XML I hacked together:

<target name="dist">
    <property name="clojure.version.label" value="clojure-1.3.0-alpha6"/>
    <property name="distdir" value="dist/${clojure.version.label}"/>
    <mkdir dir="${distdir}"/>
    <copy todir="${distdir}" includeEmptyDirs="false">
      <fileset dir="${basedir}">
        <include name="*.xml"/>
        <include name="**/*.html"/>
        <include name="**/*.txt"/>
        <include name="**/*.markdown"/>
        <include name="**/*.clj"/>
        <include name="**/*.java"/>
      </fileset>
    </copy>
    <copy file="clojure.jar" todir="${distdir}"/>
    <zip basedir="dist" destfile="clojure-${clojure.version.label}.zip"/>
  </target>

What I need is something like this, but properly parameterized and integrated into the maven build, so I can grab it from hudson or central or somewhere after a release build.



 Comments   
Comment by Stuart Sierra [ 11/Mar/11 2:24 PM ]

The POM has this set up already. Just run "mvn -Pdistribution package" to build a .zip file

This is in the README. Let me know if anything needs to be added/removed/changed.

Comment by Stuart Sierra [ 11/Mar/11 3:25 PM ]

I added "-Pdistribution" to the release build command on build.clojure.org. I think this means that the distribution ZIP file will be included in the artifacts that get uploaded to the Maven Central Repository. I don't know if this is correct, or if uploading ZIPs to Central is allowed. We'll find out the next time we do a release.

Comment by Stuart Sierra [ 03/Jun/11 8:19 AM ]

The .ZIP distribution is getting built on Hudson, but not uploaded to Sonatype or Central. It didn't really belong there anyway.

So I've configured Hudson to archive the .ZIP files on build.clojure.org.

ZIPs only get built on releases, not SNAPSHOT builds, so this should not be filling up the disk on build.clojure.org.

We can verify that this works after the next alpha release.

Comment by Aaron Bedra [ 28/Jun/11 7:08 PM ]

This worked properly with the beta release





[CLJ-769] partition-by holds references to head groups longer than necessary Created: 05/Apr/11  Updated: 21/Jun/11  Resolved: 21/Jun/11

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

Type: Defect Priority: Minor
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Clojure 1.2


Attachments: File partition-by-seq-fix.diff    
Patch: Code
Approval: Ok

 Description   

partition-by continues to hold references to items of the input sequence where it should not anymore. That is after (rest s) there are still references held to the head group of the partition-by output sequence.



 Comments   
Comment by Meikel Brandmeyer [ 06/Apr/11 1:44 AM ]

Fix for partition-by by realising the drop.

Comment by Meikel Brandmeyer [ 06/Apr/11 2:40 AM ]

I think, I finally found a way to demonstrate the issue. In the following memrem is a function, which just greedy requests memory until it is exhausted. Since SoftReference}}s are guaranteed to be cleared before an {{OutOfMemoryError is thrown, we can use this to check whether a reference to an item is retained.

user=> (def r (SoftReference. (byte-array 100000000)))
#'user/r
user=> (def s (rest (partition-by alength (list (.get r) (byte-array 100000000) (byte-array 200000000)))))
#'user/s
user=> (memrem)
.
.
java.lang.OutOfMemoryError: Java heap space (NO_SOURCE_FILE:0)
user=> (prn (.get r))
#<byte[] [B@1cfb802>
nil
user=> (def s (seq s))
#'user/s
user=> (memrem)
.
.
.
.
java.lang.OutOfMemoryError: Java heap space (NO_SOURCE_FILE:0)
user=> (prn (.get r))
nil
nil

We place a large array in a SoftReference and call partition-by such, that the array is referenced by the first group. Then we take the rest of the output sequence. Since no reference to the sequence head is retained the array should be now free for garbage collection.

However, after the memrem the SoftReference still references the array, which means there is another reference to the array. And in fact, if we actually realise the rest, this reference is gone. And after another memrem round trip the SoftReference is cleared.

The problem is that the call to drop in partition-by is not realised immediately. Hence it keeps a reference to the head of the input sequence until it is realised. However there is no reason why the drop should not immediately be realised. count is eager and hence realises the run, which in turn realises further items of the input sequence. So the elements are realised anyway. Adding a call to seq to realise the drop hence does not harm laziness of partition-by, but removes the undesired reference to the head of the input sequence. (Alternatively one could use nthnext instead of (seq (drop...)).

user=> (def r (SoftReference. (byte-array 100000000)))
#'user/r
user=> (def s (rest (partition-by-fixed alength (list (.get r) (byte-array 100000000) (byte-array 200000000)))))
#'user/s
user=> (memrem)
.
.
.
.
java.lang.OutOfMemoryError: Java heap space (NO_SOURCE_FILE:0)
user=> (prn (.get r))
nil
nil

Note, how the SoftReference is already cleared already after call to rest.

Comment by Alexander Redington [ 26/Apr/11 7:36 PM ]

Tested the patch using the following snippet to compare HEAD and the patch:

(import java.lang.ref.SoftReference)
(def r (SoftReference. (byte-array 100000000)))
(def s (rest (partition-by alength (list (.get r) (byte-array 12500000) (byte-array 1250000)))))
(defn memrem [c]
      (recur (conj c (byte-array 12500000))))
(memrem ())
(prn (.get r))

Compared with Meikel's snippets, I lowered the array sizes in partition-by's victim to fit in my default VM args.

Attached patch appears to resolve the issue consistently when measured by this method.

Comment by Fogus [ 07/Jun/11 3:27 PM ]

Ran through the logic of the change including the illustrative examples – the patch is very obscure, but cleverly done (the demonstration even more so). I considered adding a regression, but decided against it given that the nature of the illustrations are brittle with respect to the JVM settings. I would advise that a regression that does not rely on the generation of an OOME be eventually added, but that could come later unless RH objects.

Comment by Stuart Sierra [ 21/Jun/11 7:44 AM ]

Patch is in incorrect format; please use "git format-patch" as per http://clojure.org/patches

Comment by Meikel Brandmeyer [ 21/Jun/11 11:06 AM ]

Updated patch according git workflow.

Comment by Stuart Sierra [ 21/Jun/11 2:06 PM ]

Patch applied.





[CLJ-801] Protocols Should Handle Hash Collision Created: 20/May/11  Updated: 21/Jun/11  Resolved: 21/Jun/11

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

Type: Defect Priority: Major
Reporter: Christopher Redinger Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File map-fallback-cache.patch     Text File test-801b.patch     Text File test-801c.patch     Text File test-801.patch    
Patch: Code and Test
Approval: Ok

 Description   

Protocols internal is using min-hash and throwing a collision, similar to the bug fixed in CLJ-426.

https://groups.google.com/d/msg/clojure/0qllPii3fJY/Lile5yQPxuIJ



 Comments   
Comment by Alexander Taggart [ 21/May/11 9:29 PM ]

map-fallback-cache.patch: protocol's method cache falls back to using a map when shift-mask table won't work, instead of throwing an exception.

Comment by Alexander Taggart [ 27/May/11 2:45 PM ]

test-801.patch: tests method cache collision behaviour by modifying clojure.lang.MethodImplCache to allow arbitrary objects.

Comment by Fogus [ 27/May/11 3:40 PM ]

The attached test helps to illustrate the problem, but it needs some work to make it a useful regression for the existing code.

Comment by Stuart Sierra [ 01/Jun/11 1:47 PM ]

File "test-801b.patch" can replace A. Taggart's "test-801.patch" from 27/May/11.

This new test can be applied independently of the bug fix in "map-fallback-cache.patch".

The test temporarily redefines clojure.core/hash to force a hash collision. Before the bug fix, the test fails with "java.lang.IllegalArgumentException: Hashes must be distinct." After the fix, the test passes.

Comment by Alexander Taggart [ 01/Jun/11 10:36 PM ]

Just wanted to note that hash collisions are not the only issue. The shift-mask's mask is limited to 13 bits, so it's possible for hashes to be distinct but still not satisfy the constraints. This latter condition is what was being tested by the "no min-hash" test case in test-801.patch.

Comment by Stuart Sierra [ 02/Jun/11 8:10 AM ]

I'll try to add another test for the no-min-hash case.

Comment by Stuart Sierra [ 02/Jun/11 9:15 AM ]

File "test-801c.patch" supplants "test-801b.patch" and adds another test for the no-min-hash case.

Both tests fail before "map-fallback-cache.patch" and pass after.

Comment by Tassilo Horn [ 14/Jun/11 2:45 AM ]

I've just applied map-fallback-cache.patch on top of commit 66a88de9408e93cf2b0d73382e662624a54c6c86, and now my project runs fine. Before, I frequently had these "no distinct mappings found" error.

Comment by Stuart Sierra [ 21/Jun/11 7:41 AM ]

Code & Test patches pushed.





[CLJ-800] defrecord/deftype enhancements from 1.3.0-alpha7 Created: 18/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Enhancement Priority: Major
Reporter: Fogus Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-800-defrecord-deftype-fixes-from-1.3.0-alpha7.patch     Text File CLJ-800-defrecord-deftype-fixes-from-1.3.0-alpha7.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

The defrecord read/print functionality added in clojure-1.3.0-alpha7 based on ticket CLJ-374 provided a baseline semantics for the described functionality. However, there were some short-comings of the implementation that should be fixed, including:

  • Symbols not supported in the record/type reader form
  • Type forms are not performant as a result of being handled by print-dup
    • This also leads to a situation where hinting is problematic
  • The boxClass method uses Reflector
  • Emission of records pulls out k/v's to create new map to build new record. Record itself should be used instead
  • Factory fn should not restrict definition of record/type with >20 fields (see here)

The semantics of the 1.3.0-alpha7 behavior should remain intact (save for bug fixes that open more functionality).



 Comments   
Comment by Fogus [ 18/May/11 7:09 AM ]

Added patch with fixes and tests.

Comment by Fogus [ 24/May/11 7:29 AM ]

Updated patch to account for records/types of >20 fields. (see here)

Comment by Stuart Halloway [ 27/May/11 11:51 AM ]

rest args such as overage in build-positional-factory are not clojure.lang.Indexed. Possible perf issue for (n - 20) calls to nth creating large records?





[CLJ-797] Longs print-dup to the Long constructor Created: 16/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Minor
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File print-dup-longs.patch     Text File print-dup-longs-update-1.patch    
Patch: Code and Test
Approval: Ok

 Description   
user=> (binding [*print-dup* true] (print-str 1.0))
"1.0"
user=> (binding [*print-dup* true] (print-str 1))
"#=(java.lang.Long. \"1\")"


 Comments   
Comment by Alexander Taggart [ 27/May/11 1:40 PM ]

print-dup-longs-update-1: apply after CLJ-798





[CLJ-802] mod throws exception on large args Created: 22/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Minor
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix-mod.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

From http://groups.google.com/group/clojure-dev/browse_thread/thread/09718dc9c253d1ab

user=> (mod 3216478362187432 432143214)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1374)





[CLJ-789] Method resolution does not select exact signature matches over tying Created: 10/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Critical
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File 789.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

Given 3+ Java methods:

public static boolean someMethod (int a, int b) {
    return true;
}

public static boolean someMethod (int a, long b) {
    return true;
}

public static boolean someMethod (long a, long b) {
    return true;
}

and a Clojure call that matches one of them exactly:

(SomeClass/someMethod (long 1) (long 1))

the compiler yells at us:

More than one matching method found: someMethod

It turns out that Compiler.getMatchingParams is not clearing its "tied" flag when a later signature is an exact match.



 Comments   
Comment by Chas Emerick [ 10/May/11 8:24 AM ]

patch attached





[CLJ-374] print/read syntax for defrecords Created: 05/Jun/10  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Fogus
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File CLJ-374-defrecord-literals.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Comments   
Comment by Assembla Importer [ 24/Aug/10 9:32 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/374

Comment by Assembla Importer [ 24/Aug/10 9:32 AM ]

stu said: If we are going to use #=, then how about modify record to take an optional metadata map before the keyvals:

(defn record
  "Construct an instance of record-type from keyvals.

   record-type is a keyword naming the record.
   keyvals can include *any* keyword/value pairs, and
   can be preceded by an optional metadata map."
  {:added "1.2"}
  [record-type & keyvals]
  {:pre [(keyword? record-type)]}
  (let [meta (when (map? (first keyvals)) (first keyvals))
        keyvals (if meta (rest keyvals) keyvals)]
    (record-implementation-detail-multimethod record-type meta (apply hash-map keyvals))))
</code></pre>

and then the read syntax can be

<pre><code>
#=(record :user.Foo {:some-meta 1} :a nil :c 3)
Comment by Assembla Importer [ 24/Aug/10 9:32 AM ]

richhickey said: I think you need to distinguish between print and print-dup contexts. I don't like the switch-on-type-of-first-arg in general. We don't do optional args preceding variadics. Also, normally we don't see metatadata in print unless print-meta is true.

I think a #= ctor call works for print-dup:

#=(Foo. 1 2 3 {:my :meta} {:other :field})

eliding the meta and extmap when neither is present:

#(Foo. 1 2 3)

not sure about ordinary print yet

Comment by Assembla Importer [ 24/Aug/10 9:32 AM ]

stu said: In the above, why isn't print-dup's effect recursive, e.g.

#=(user.Foo 1 2 3 #=(clojure.lang.PersistentArrayMap/create {:my :meta}) 
                                 #=(clojure.lang.PersistentArrayMap/create {:other :field}))

print-dup on maps certainly behaves that way, including nested maps.

Also, isn't a print-dup version that includes metadata only correct when print-meta is false?

Comment by Assembla Importer [ 24/Aug/10 9:32 AM ]

stu said: Updating tickets (#370, #366, #374)

Comment by Fogus [ 29/Apr/11 1:30 PM ]

Patch scoped to the discussion in this ticket and at http://dev.clojure.org/display/design/defrecord+improvements

Comment by Fogus [ 29/Apr/11 1:55 PM ]

Added IRecord patch file. Sorry.

Comment by Stuart Halloway [ 29/Apr/11 3:14 PM ]

Using primitive type hints seems to break the map->Record constructor:

(defrecord Bar [^long a b])
(map->Bar {:a 1 :b 2})
=> NullPointerException   clojure.lang.RT.longCast (RT.java:1008)
Comment by Stuart Halloway [ 29/Apr/11 3:21 PM ]

The patch does not address deftype. It gets (as a freebie) all the new stuff available to arbitrary Java classes, but no create method, positional fn, etc. I think we should do deftype as a separate patch.

Comment by Fogus [ 29/Apr/11 3:45 PM ]

RE: longCast

I will check that out. Thanks

RE: deftype

deftype does get a create and getBasis method, but the create is breaking because I am assuming a record ctor. I can fix that if we think that types should have a create that takes a map.

(deftype T [a])
(user.T/getBasis)
;=> [a]

(user.T/create {:a 1})
;; NoSuchMethodError

This is because I'm building a call to the record ctor that takes metadata and an extension map. Does it make sense to generate such a create that takes a map for types?

Thanks
:F

Comment by Fogus [ 05/May/11 5:25 PM ]

minor fixes from last patch set.

Comment by Rich Hickey [ 06/May/11 7:50 AM ]

deftypes shouldn't get any map-taking support as they are not mappy (mappish?). They should get positional support. defrecord and deftype stuff should go in together.

Comment by Fogus [ 10/May/11 7:20 AM ]

Latest patch includes changes to support deftype literals and removal of RecordExpr.

Comment by Christopher Redinger [ 25/May/11 2:25 PM ]

This patch has already been applied. Updating status of this ticket.

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





[CLJ-671] Recur mismatch causes infinite loop in compiler Created: 05/Nov/10  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Major
Reporter: Juha Arpiainen Assignee: Fogus
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 671-recur-mismatch-infinite-loop.diff     File 671-recur-mismatch-infinite-loop-test.diff    
Approval: Ok
Waiting On: Rich Hickey

 Description   

In cases like

(set! warn-on-reflection true)

(defn gcd [x y]
(loop [x (long x) y (long y)]
(if (== y 0)
x
(recur y (rem x y)))))

I'd expect either both x and y to be auto-boxed or an error from the primitive local mismatch.
Instead the compiler (1.3.0-alpha2) goes into an infinite loop in LetExpr.Parser.parse():

NO_SOURCE_FILE:3 recur arg for primitive local: y is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: y
NO_SOURCE_FILE:3 recur arg for primitive local: x is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: x
NO_SOURCE_FILE:3 recur arg for primitive local: y is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: y
NO_SOURCE_FILE:3 recur arg for primitive local: x is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: x
.
.
.

The previous case has been fixed as described in the comments. The defect can still be seen this way:

(set! warn-on-reflection true)

(defn gcd [x y]
(loop [x (long x) y (long y)]
(if (== y 0)
x
(recur y ^Long(rem x y)))))



 Comments   
Comment by Juha Arpiainen [ 05/Nov/10 3:33 PM ]

Attached patch (CA is now in mail).

With this patch the loop body will be analyzed n times in cases like

(loop [v1 0 v2 0 v3 0 vn 0]
(recur v2 v3 vn (identity vn)))

Comment by Stuart Halloway [ 15/Nov/10 12:55 PM ]

The long casts are not necessary here, but if they are present, they appear to cause an infinite loop regardless of whether warn-on-reflection is set.

Comment by Alexander Taggart [ 03/Mar/11 5:03 PM ]

Test case no longer results in erroneous behaviour using current master branch.

Comment by Christopher Redinger [ 15/Apr/11 11:00 AM ]

Confirmed that this is no longer an issue on alpha-6

Comment by Juha Arpiainen [ 15/Apr/11 11:44 AM ]

Test case works because of improved type inference for #'rem. Changing (rem x y) to ^Long (rem x y) in test case still causes the loop in f0a46155ba3...

Comment by Christopher Redinger [ 15/Apr/11 11:58 AM ]

Updating test case to show present day failure

Comment by Christopher Redinger [ 28/Apr/11 9:02 AM ]

Jahu is resending a CA, we did not receive the first one.

Comment by Fogus [ 29/Apr/11 2:03 PM ]

Added regression for this patch. Waiting on CA for final approval.

Comment by Christopher Redinger [ 27/May/11 10:47 AM ]

We've received the CA. This patch is ready to go.





[CLJ-794] Some alpha-7 printdup functions have left-over debug messages. Created: 13/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Major
Reporter: Fogus Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Clojure alpha7


Attachments: Text File CLJ-794-leftover-debug-msgs.patch    
Patch: Code
Approval: Ok
Waiting On: Stuart Halloway

 Description   

(defmethod print-dup clojure.lang.IPersistentCollection [o, ^Writer w] (print " ipcpd ")
(defmethod print-dup clojure.lang.PersistentHashMap [o w] (print " phmpd ") (print-method o w))






[CLJ-795] bit-clear index should be zero based Created: 15/May/11  Updated: 27/May/11  Resolved: 27/May/11

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

Type: Defect Priority: Major
Reporter: Christopher Redinger Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-795.patch    
Patch: Code and Test
Approval: Ok

 Description   

As posted to the mailing list: https://groups.google.com/d/topic/clojure/ky_cVkQyhNw/discussion

alpha7 introduced a regression where the bit-clear index is no longer zero based

user> clojure-version
{:major 1, :minor 3, :incremental 0, :qualifier "alpha4"}
user> (bit-clear 3 1)
1
user> (bit-clear 3 0)
2
user> (bit-clear 3 2)
3
user>
user> clojure-version
{:major 1, :minor 3, :incremental 0, :qualifier "alpha7"}
user> (bit-clear 3 1)
2
user> (bit-clear 3 0)
1
user> (bit-clear 3 2)
0



 Comments   
Comment by Alexander Taggart [ 16/May/11 2:23 AM ]

Patch fixes regression and adds some tests.





[CLJ-772] bit ops to have primitive semantics by default, no conditionals, direct mapping to JVM primitive ops Created: 07/Apr/11  Updated: 06/May/11  Resolved: 06/May/11

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

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File bit-ops.patch    
Patch: Code and Test
Approval: Ok

 Description   

Per Rich's comment on CLJ-767.



 Comments   
Comment by Alexander Taggart [ 07/Apr/11 8:12 PM ]

Waiting on confirmation that patch on CLJ-767 is correct.

Comment by Stuart Halloway [ 29/Apr/11 3:54 PM ]

commit message is odd, but commit looks right





[CLJ-782] long cast is not checked for Object decimal types Created: 28/Apr/11  Updated: 06/May/11  Resolved: 06/May/11

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

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File check-longs.patch     Text File check-longs-update-1.patch    
Patch: Code and Test
Approval: Ok

 Description   

E.g.:

user=> (*' Long/MAX_VALUE 100M)
922337203685477580700M
user=> (long *1)
-100
user=> (Double/valueOf Double/MAX_VALUE)
1.7976931348623157E308
user=> (long *1)
9223372036854775807

And the numbers.clj test erroneously considers truncation as correct.






[CLJ-426] case should handle hash collision Created: 13/Aug/10  Updated: 29/Apr/11  Resolved: 29/Apr/11

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

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 426.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

should generate a branch under the hash to deal with the conflicting values



 Comments   
Comment by Assembla Importer [ 29/Sep/10 9:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/426

Comment by Assembla Importer [ 29/Sep/10 9:30 AM ]

stu said: Updating tickets (#427, #426, #421, #420, #397)

Comment by Assembla Importer [ 29/Sep/10 9:30 AM ]

cemerick said: It would be nice if case were further enhanced to support references to enums and static final fields (and maybe other stably-referenceable values as well?). A userland workaround is shown here (added here instead of in a separate ticket at Rich's request).

Comment by Rich Hickey [ 07/Jan/11 7:40 AM ]

while in there we should look at special-casing all-int case, and primitive return from case

Comment by Aaron Bedra [ 09/Jan/11 2:38 PM ]

I'm gonna give this a shot. If anyone has any solutions to this please feel free to jump in here while I am working on it.

Comment by Alexander Taggart [ 28/Feb/11 12:59 PM ]

case changes:

  • handles hash collisions
  • can emit return type
  • conditionally supports java enums
  • performance path for all-int or all-char test constants
Comment by Alexander Taggart [ 01/Mar/11 12:02 AM ]

Patch happens to fix CLJ-438 as well.

Comment by Alexander Taggart [ 02/Mar/11 5:05 AM ]

Updated patch so that case is no longer one gigantic ball of code. And fixed some bugs

Comment by Alexander Taggart [ 02/Mar/11 5:28 AM ]

Update to fix a bug.

Comment by Alexander Taggart [ 02/Mar/11 7:55 PM ]

Per cemerick's request I'm providing a secondary patch which would provide for evaluating test-constant symbols at compile time. This would allow us to use switches where writing a literal value is difficult. E.g.:

user=> (defn foo [x]
         (case x
           java.lang.Math/PI :pi
           java.lang.Math/E :e
           :oops))
#'user/foo
user=> (foo java.lang.Math/E)
:e

Quoted symbols are not evaluated, but there are backwards compatibility concerns with evaluating unquoted symbols:

  • if evaluation is successful, it might be unintended thus breaking existing code
  • if evaluation is unsucessful
    • continuing to treat it as a symbol would preserve partial backwards compatibility
    • throwing an error would break compatability, but allow for a more consistent behavior going forward, i.e., quote symbols when you want symbols
Comment by Stuart Halloway [ 04/Mar/11 2:52 PM ]

What's the basis for deciding when to use tableswitch vs. lookupswitch? I tried the sparse example [-100, 0, 100] from http://java.sun.com/docs/books/jvms/second_edition/html/Compiling.doc.html and the code produced a tableswitch. Is this desirable?

Comment by Stuart Halloway [ 04/Mar/11 2:54 PM ]

Rich: screened in this case means only that the tests pass. I am continuing to dig into this patch, adding comments as I go. Let me know if there are specific things I should be investigating.

Comment by Stuart Halloway [ 04/Mar/11 2:59 PM ]

enum support feels half-baked. For example, an attempt to mix enums and non enums compiles but then cannot find the enum:

(case clojure.lang.Compiler$CaseTestEnumABC/a
        clojure.lang.Compiler$CaseTestEnumABC/a 1
        1 2)
IllegalArgumentException No matching clause: a
Comment by Stuart Halloway [ 04/Mar/11 3:08 PM ]

The use of eval to detect enums feels icky, and counter to the stated behavior of allowing only compile-time constants.

Comment by Stuart Halloway [ 04/Mar/11 3:12 PM ]

Rich: to the extent that we believe in testing for things like reflection warnings, it would be nice to have a way to get to that information other than be scraping stderr. If this is of interest let me know and I will make a ticket and/or start a design discussion as appropriate.

Comment by Alexander Taggart [ 04/Mar/11 4:45 PM ]

Regarding the mixed enum/non-enum case:

The primary patch treats symbols which do not resolve to enums as symbols, but also treats enums as symbols when the set of test-constants was not all-enum. Both parts were an attempt at backwards compatibility.

I agree that the better behaviour would be to error on the mixed case, though this would break any code where a test-constant symbol happens to resolve to an enum.

Regarding eval:

It doesn't get around the "compile-time" restriction, but rather the "literal" restriction.

Comment by Rich Hickey [ 04/Mar/11 5:03 PM ]

Unless someone can provide some simple semantics for enum support, I'd rather not support them.

Re: lookupswitch - I don't see any reason to support it - is the tableswitch code in some way wrong?

Comment by Alexander Taggart [ 04/Mar/11 7:39 PM ]

simple semantics for enum support

As the updated doc states: "Java enums are supported if all test constants are enums of the same type."

Stu's example violates the above rule. The issue then becomes what should be the response: error at compile-time, or treat as symbol. More on this in a following comment.

Re: lookupswitch ... is the tableswitch code in some way wrong?

Not at all, but the current version will throw an exception when it can't find a sufficient shift-mask:

user=> (case 1
         1     :a
         16384 :b
         16385 :c)
IllegalArgumentException No distinct mapping found  clojure.core/min-hash (core.clj:5708)

The lookupswitch bytecode is only used to substitute for throwing that exception.

The current process of fitting test constants into a switch:

  1. Coerce to int by hashing
  2. if shift-mask found, use tableswitch and shift-mask
  3. else throw exception

The process used in the patch:

  1. Coerce to int (hash, enum ordinal, or int value)
  2. if (max-int - min-int) < max-tableswitch-size, use tableswitch without shift-mask
  3. else if shift-mask found, use tableswitch with shift-mask
  4. else use lookupswitch without shift-mask

Where max-tableswitch-size is simply an explicit consequence of the mask width.

Comment by Rich Hickey [ 04/Mar/11 7:59 PM ]

Java enums aren't Clojure constants.

Comment by Alexander Taggart [ 04/Mar/11 9:50 PM ]

Support for enums is really just a degenerate case of supporting named constant values, e.g., java.lang.Math/PI, (def init-state 0).

The basic requirement of case is that the test constants have a known value at compile-time, and that value has consistent hashing. The current implementation assumes the latter requirement will be met by restricting test constants to literal types. This mostly works, but fails with things like regex patterns.

The above basic requirements can be met while removing the incidental "literal" restriction. To do so means resolving literal symbols into constant values at compile-time, and then validating that the types are suitable (e.g., number, string).

The simplest and most consistent approach would be to require quoting test constant symbols when used as symbols. Setting aside all other discussion of enums, etc., is this change to the treatment of symbols acceptable?

Comment by Alexander Taggart [ 04/Mar/11 10:39 PM ]

To make life easier, I'm altering the set of patches.

426-basic.patch:

  • handles hash collisions
  • can emit return type
  • performance path for all-int or all-char test constants
  • no documentation change

426-supports-named-values.patch:

  • all from above, plus...
  • evaluates unquoted test constant symbols at compile-time
  • validates test constants are suitable for case
  • doc change: "The test-constants can be any combination of numbers, chars, strings, keywords, quoted symbols, and (Clojure) composites thereof. Unquoted symbols will be evaluated at compile time, and must resolve to one of the preceding types."

426-supports-enums-and-named-values.patch:

  • all from above, plus...
  • conditionally supports java enums
  • doc change: "Java enums are supported only if all test constants are enums of the same type."
Comment by Chas Emerick [ 05/Mar/11 7:33 AM ]

RH:

Java enums aren't Clojure constants.

SH:

The use of eval to detect enums feels icky, and counter to the stated behavior of allowing only compile-time constants.

I assume both of you are actually talking about literals. Limiting case to literal test values is simply too much of a handicap, even ignoring interop uses. One should not have to macro their way out of repeating a constant value (e.g. (def *mole* 6.02214179e23)) throughout a codebase that needs to use case extensively.

Leaving aside questions about mechanics, static final fields and enums are absolutely constants, and not being able to use them within case would be (and is today) a significant pain point. Yes, their value w.r.t. case can change after a case form has been AOT-compiled – which exactly mirrors switch (a good thing, IMO, making case a proper superset of switch). Again, something one can macro out of, but something I don't think people should have to macro out of.

Comment by Rich Hickey [ 05/Mar/11 7:35 AM ]

Thanks for splitting this up. I'm not in favor of anything beyond basic. The enum stuff is a snake pit full of new semantics, contradictions and special cases. I had told Chas as much. Sorry I hadn't realized the enum thing had crept into this scope sooner.

Rich

Comment by Chas Emerick [ 05/Mar/11 7:36 AM ]

I should say that I'd consider it perfectly reasonable if case were kept limited to using literals (if that limitation is considered important enough to retain at some level), but clojure.core provided a switch macro that evaluated symbols as Alexander has been reaching for.

Comment by Rich Hickey [ 05/Mar/11 8:04 AM ]

Chas, I'm not sure what you mean by 'macro their way out of..." - are you saying def is too much work? Or is what you'd want, but doesn't work?

What's missing is not some extension to case, but possibly a broadening of what constitutes a compile time constant. At no point should that involve compile time evaluation in case. A defconst has long been requested and is a fine idea. Ditto static finals as constants. As long as case is defined to work with constants, we can enhance 'constants' and case can do more transparently (at least semantically transparently). All of this stuff around unquoted constant symbols, conditional compile time evaluation (when does that otherwise happen?) etc is a mess. Note that case is currently defined in terms of literals because Clojure doesn't have a strong notion of constant yet.

Also, I think you are not adequately considering the use of case in symbolic computation. People use case matching of symbols all the time when writing macros and compilers etc in CL, and wouldn't appreciate having to quote everything. Also, given we can match aggregates, how might one match (quote x), and why wouldn't that match 'x?

This is scope creep, and insufficiently considered, plain and simple. Let's leave it as a separate ticket. It's going to require more thought, in any case

Comment by Chas Emerick [ 05/Mar/11 1:07 PM ]

Well, everything is scope creep in the end.

By "macro their way out", I meant doing something like I did here to use non-literal constants in case forms. When I wrote that comment this morning, I was under the impression that only Clojure literals were considered acceptable.


Anyway, what follows is either incredibly pedantic, or a draft of a wiki page where a more agreeable solution can be hashed out.

It occurs to me that being explicit about what constants exist and what's on the table here would be helpful, at least for me:

  1. Clojure literals and composites thereof, as implemented in 1.2.0 case
  2. static final fields
  3. def'ed values, presumed to be unchanging
  4. enums

(1) is granted, and it sounds like moving beyond this is not desired at this time without significant further analysis/discussion (implying the application of Alexander's "basic" patch only).

(2) appears to be desired ("Ditto static finals as constants"). However, as soon as we allow for non-literal constant values (whether fields, def's, enums, or other), I'm very confused as to:

  • how what we use to refer to those values (surely symbols?) aren't turned into a second-class test values
  • how we're not faced with conditional resolution of those values (there's no calls anywhere, so it seems there's little difference between eval and direct usage of Reflector, at least for fields and enums)

Perhaps the defconst variation hinted at in conjunction with (3) would somehow avoid compile-time evaluation for def'ed values? I'm no longer familiar with CL and the defconstant prior art to reasonably guess at the relevant semantics of defconst.

Re: (4) and "The enum stuff is a snake pit full of new semantics, contradictions and special cases": I'm pretty sure I didn't know that. It seems like enum support is as close as:

(.ordinal (Reflector/getStaticField EnumClassName "EnumName"))

…with all reasonable error-checking and such to ensure the domain of the case form includes only enums of the same type (though again, the issues with resolving symbols remains).


Anyway, I'm sorry to have encouraged (egged on?) Alexander. I appear to have misread/misunderstood the desired scope of changes.

Comment by Alexander Taggart [ 05/Mar/11 2:04 PM ]

Chas, no worries. Being able to use named constant values as test constants makes my life so much easier, especially when dealing with java interop. Until now I've had to use (condp = x ...). Regardless of whether it makes it into core, I will still be using it.

Comment by Rich Hickey [ 06/Mar/11 11:05 AM ]

Chas -

1) Is all we've got right now. Literals compile into themselves. There is no notion of constant in the language.

2) Treating finals as constants would require the compiler compile MyClass/MY_FINAL into its value, which it does not currently do. It currently generates a getStaticField opcode.

3) I never suggested treating defs like constants. A defconst would require some notion of constant. Since its value would be compiled into code, it must be different from def, the semantics of which are to compile into a deref of the var. Only the latter currently occurs. Doing otherwise automagically just inside case would be bizarre. Languages typically constrain the initializers of such constants, esp. to constant expressions themselves. CL allows for arbitrary initializers, but requires they be eval-able at compile time, always evaluate to the same value, and with much hand waving about execution order.

e.g. if as you had suggested:

(def x (launch-missiles))

(case foo x ...)

Would missiles be launched during compilation or loading or both? Would x be nil or the number of missiles launched in the case? What if the case were wrapped in (binding/let [x ...] ...)?

4) is like 2.

You are correct, 2,3,4 all conflict with the use of symbols in case, which is why supporting them is a breaking change, glommed on to a ticket that nominally is a bug fix, augmented by a perf optimization.

Fixing the conflict isn't as simple as saying "use 'x for symbols" in case. You'd need to consider the interaction of constants and all special forms and macros, the relationship between constants and '[un]evaluated' positions everywhere. It's not a trivial thing. CL, FWIW, does not in fact support its own defconstants as keys in its case, considering them constants only in evaluated positions.

I don't have the time right now to consider all of that, but I won't accept a design that hasn't.

Comment by Stuart Halloway [ 06/Mar/11 12:03 PM ]

Reviewing the basic patch. The code passed to case* is incorrect for some duplicate hash scenarios. One example:

(let [f #(case % 0 :ZERO -1 :NEG1 2 :TWO :OOPS :OOPS)]
    (doseq [input [0 -1 2 :OOPS]]
      (println (format "(f %s) = %s" input (f input)))))
(f 0) = :ZERO
(f -1) = :NEG1
(f 2) = [2 :TWO]
IllegalArgumentException No matching clause: :OOPS

It would be a huge help to me in screening if the docstrings for the private helper fns documented expected inputs and outputs.

Comment by Alexander Taggart [ 06/Mar/11 6:25 PM ]

Hash-collision handling function did not properly handle non-colliding test/then entries.

426-basic-update-1.patch fixes the above and adds Stu's example to the test suite.

Comment by Alexander Taggart [ 06/Mar/11 6:26 PM ]

426-basic-update-1.patch also contains additional documentation to the various helper functions.

Comment by Stuart Halloway [ 11/Mar/11 11:43 AM ]

Alex: There is something weird about characters:

(case (char \uFFFF) \uFFFF 1)
=> 1
(case \uFFFF \uFFFF 1)
=> IllegalArgumentException No matching clause: ?  user/eval2253 (NO_SOURCE_FILE:126)
(case \a \a 1)
=> IllegalArgumentException No matching clause: a  user/eval2256 (NO_SOURCE_FILE:127)
(case (char \a) \a 1)
=> 1

(These all return "1" prior to the patch.)

Comment by Alexander Taggart [ 11/Mar/11 2:51 PM ]

Good catch, Stu.

Causes:

  • character literals are analyzed as ConstantExprs which do not emit primitives, thus the case code went into the "hash" branch.
  • character test values (not the case values) were erroneously ints, so the post-switch validation looked like (= \a 97).
    • this also meant there was equivalence between numbers and chars, which is not consistent with the rest of clojure.

Fix:

426-basic-update-2.patch: removes support for the all-chars case

Aside:

Would it be reasonable to have a CharExpr analogue to NumberExpr?

Comment by Stuart Halloway [ 20/Mar/11 9:31 AM ]

some inputs (which formerly causes "no distinct mapping found" can now cause negative array access or exhaust memory

;; negative array size exception
(defn foo [x]
  (case x
        -1993159583054299157 -1993159583054299157
        -4 -4
        0 0
        -1028157349872421032 0
        :nwuhzgv1k4Gl8eyE*+4UT0b1wIlN!ohzv3!snZ-dN6TBWZ7aOpCYjk3cwgKUHDenjBkx*dwIudNqXVHPDSyuB8yE!d1dSDDGGi5_v5FwC+S_Pr+?hXmEdiL2_3ND+_UCVY4IH8bUw 0
        2 2))


;; out of memory
(defn foo [x]
  (case x
        -1993159583054299157 -1993159583054299157
        -4 -4
        0 0
        :nwuhzgv1k4Gl8eyE*+4UT0b1wIlN!ohzv3!snZ-dN6TBWZ7aOpCYjk3cwgKUHDenjBkx*dwIudNqXVHPDSyuB8yE!d1dSDDGGi5_v5FwC+S_Pr+?hXmEdiL2_3ND+_UCVY4IH8bUw 0
        2 2))
Comment by Alexander Taggart [ 20/Mar/11 1:44 PM ]

Both of the above were due to label array creation not taking into account the type of switch being emitted. Fixed in 426-basic-update-3.patch.

Comment by Alexander Taggart [ 21/Mar/11 4:38 PM ]

426-basic-update-4.patch update to apply to current master branch.

Comment by Stuart Halloway [ 05/Apr/11 8:21 PM ]

update-4 fails for this input:

(defn foo
  [x]
  (case x
        -1 -1))

Maybe an issue with case statements that are all negative ints?

Comment by Alexander Taggart [ 06/Apr/11 3:41 AM ]

Update 5 fixes the above bug.

In the previous patch, if all the test expressions were ints but the tested expression was not known to be primitive, the hashcode branch was used. The hashcode of Integers is equal to their value, but for Long's that's only true for the positive range. This was the source of the bug you found, namely the value in the switch (-1) didn't match the hash of Long -1 (0).

Instead of reusing the hashcode branch, bytecode like the following is now emitted:

if (expr instanceof Number)
  switch(((Number)expr).intValue()){
    case -1:
      if (Util.equiv(expr, -1))
        return -2;
  }
// default
throw new IllegalArgumentException();

Note that the performance warning is still in place, albeit with a better message:
"Performance warning, %s:%d - case has int tests, but tested expression is not primitive.\n"

Comment by Christopher Redinger [ 15/Apr/11 12:55 PM ]

Please Test patch

Comment by Christopher Redinger [ 21/Apr/11 12:17 PM ]

FYI: patch applies cleanly & tests pass against master as of 4/21 (2011)

Comment by Stuart Halloway [ 22/Apr/11 11:48 AM ]

I am seeing failure for the following input:

(defn foo
 [x]
 (case x
   1204766517646190306 9
   1 8 
   -2 6))

Sorry for the screwy formatting in JIRA.

Comment by Alexander Taggart [ 22/Apr/11 2:14 PM ]

Updated patch to fix above.

Explanation of bug:

When multiple test constants have a hash collision, the respective thens are combined with a condp, and the colliding hash value is used as the test constant. Since the combined then performs its own post-switch validation, the normal (non-colliding) behaviour of checking that the original test value equals the tested expression must be skipped.

The bug occurred because the skip-check set of case ints was not getting the necessary shift-mask applied to it.

The bug was not caught earlier because there was incomplete test coverage, namely, for the case of both hash collision and shift-mask application.

Comment by Stuart Halloway [ 26/Apr/11 8:01 PM ]

code now passes test.generative tests. Test run shows case to be faster than cond (and test code could be improved to more directly compare speed).

(binding [*msec* 100000]
  (doseq [v (test-namespaces 'clojure.test.core-test)]
    @v))

{:iterations 2204, :msec 33339, :spec #'clojure.test.core-test/cond-spec, :seed 43}
{:iterations 2159, :msec 33353, :spec #'clojure.test.core-test/cond-spec, :seed 42}
{:iterations 3298, :msec 33339, :spec #'clojure.test.core-test/case-spec, :seed 43}
nil
{:iterations 3217, :msec 33338, :spec #'clojure.test.core-test/case-spec, :seed 42}

This is a fairly large patch – please advise if we need to find a way to break into into smaller chunks, or if there are other perf tests you would like to see.





[CLJ-236] Add error checking for defmulti options. Created: 01/Jan/10  Updated: 29/Apr/11  Resolved: 29/Apr/11

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0236-defmulti-error-checking-with-helper.patch     Text File 0236-defmulti-error-checking-with-helper.patch    
Approval: Ok
Waiting On: Rich Hickey

 Description   

It would be nice to have some error checking for option map in macros like defmulti. I've written a check-options function that can be used for that purpose and used it in defmulti definition in the attached patch.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 6:44 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/236
Attachments:
0001-Added-the-check-options-function-to-warn-about-wrong.patch - https://www.assembla.com/spaces/clojure/documents/cD6KBS90Kr3RGweJe5afGb/download/cD6KBS90Kr3RGweJe5afGb

Comment by Assembla Importer [ 28/Sep/10 6:44 AM ]

richhickey said: Thanks for this. Should throw IllegalArgumentException I think.

Comment by Stuart Halloway [ 17/Dec/10 3:01 PM ]

Rich, I have a few design questions before I edit this patch:

  • For macros throwing IllegalArgumentException looks right, but I would want basically the same fn as a precondition assertion in normal code.
  • To the extent that the previous argument (or any other kind of option checking) is persuasive, I am reluctant to give out the generic name "check-options" in core without more thought. Maybe make this private for now? Or call it check-macro-options? Put in a different namespace?
Comment by Rich Hickey [ 04/Jan/11 8:19 PM ]

I don't understand

Comment by Stuart Halloway [ 28/Jan/11 12:45 PM ]

I have two ideas for this patch:

  1. the thrown error tells you what you should have done (the valid keys) but not what you did (the keys you passed). Should do both.
  2. the name check-options. There might be an opposite fn (make sure you did provide keys) that could claim the same name. Maybe constrain-options? Or, to the extend that it works with any map, could be constrain-keys. ... Which suggests the partner fn require-keys.

Thoughts?

Comment by Stuart Halloway [ 22/Apr/11 9:36 AM ]

Rich: we had discussed making the validation helper a local fn, but I like the idea of making it private instead. I expect there will be other uses in the future. Hope that is ok. If not, let me know and I will make the patch we discussed.





[CLJ-435] stackoverflow exception in printing meta with :type Created: 14/Sep/10  Updated: 29/Apr/11  Resolved: 29/Apr/11

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0435-printing-badly-typed-objects.patch    
Approval: Ok

 Description   

Hi, after expose the problem in clojure irc and the help of @chouser, we thought my problem must be a bug.
Here the console session

user> (with-meta {:value 2} {:type Object})

No message. [Thrown class java.lang.StackOverflowError]

0: clojure.lang.PersistentHashMap$BitmapIndexedNode.index(PersistentHashMap.java:485)
1: clojure.lang.PersistentHashMap$BitmapIndexedNode.find(PersistentHashMap.java:571)
2: clojure.lang.PersistentHashMap$ArrayNode.find(PersistentHashMap.java:355)
3: clojure.lang.PersistentHashMap.entryAt(PersistentHashMap.java:129)
4: clojure.lang.Var.getThreadBinding(Var.java:334)
5: clojure.lang.Var.deref(Var.java:137)
6: clojure.lang.Var.get(Var.java:133)
7: clojure.core$pr_on.invoke(core.clj:2810)
8: clojure.lang.Var.invoke(Var.java:369)
9: clojure.lang.RT.print(RT.java:1270)
10: clojure.lang.RT.printString(RT.java:1249)
11: clojure.lang.APersistentMap.toString(APersistentMap.java:20)

Chouser pointed that is a print error, creation and use of map with meta works fine

Thank for your help!



 Comments   
Comment by Assembla Importer [ 08/Oct/10 10:36 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/435

Comment by Assembla Importer [ 08/Oct/10 10:36 AM ]

chouser@n01se.net said: The problem is the print-method for Object calls .toString on the object, clearly expecting some nice tame Java default implementation. But in this case, the object is actually APersistentMap, which has a .toString that uses print-method. Oops.

One possible resolution is simply to say: when you lie about an object's type, bad things can happen. :type Object is essentially claiming that the map's concrete type is Object and that's simply not true. Perhaps breakage should be expected.

On the other hand, if we want to fail more gracefully either with a clearer error or "pre-initialized" print defined in RT.print, I'm not sure I see a solution other than explicit recursion detection. In this case it could take the form of a private Var set to the object being printed. If the Var is already equal to the object you're supposed to print, you're recursing on a single object and need to break out somehow.

Can anyone think of a cleaner way to catch this?

Comment by Meikel Brandmeyer [ 21/Mar/11 1:53 PM ]

Maybe one could check that (= (type thing) (class thing)) if the result of type is a Class? Types in meta data should normally be Keywords, no?

Comment by Stuart Halloway [ 22/Apr/11 9:13 AM ]

I chose to make the minimal change to print-method, rather than changing the type function, which would have been a breaking change to documented behavior.

Aside: Should type be deprecated?





[CLJ-774] Message-bearing assert Created: 15/Apr/11  Updated: 29/Apr/11  Resolved: 29/Apr/11

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

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Attachments: File 0744-message-carrying-asserts.diff    
Patch: Code
Approval: Screened
Waiting On: Stuart Halloway

 Description   

assert should take a String as a second argument. The string is printed when the assert fails.

This is lower hanging fruit than CLJ-415 but much better than the assert we currently have.

In the future, the second argument might be an expression that gets evaluated, but for now, it shouldn't be.



 Comments   
Comment by Rich Hickey [ 29/Apr/11 8:00 AM ]

If someone is supplying a message, will they want the original message like that?





[CLJ-686] create JIRA workflow picture Created: 03/Dec/10  Updated: 22/Apr/11  Resolved: 22/Apr/11

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Christopher Redinger
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

... and post and link from appropriate places



 Comments   
Comment by Christopher Redinger [ 03/Dec/10 4:23 PM ]

Moved to a google doc

https://docs.google.com/document/d/1PiJ4O2ZGWfReKNcG5ON8zrRZVYsFQiKrQRoduAK6upQ/edit?hl=en&authkey=CL7U4ugD

Comment by Stuart Halloway [ 13/Dec/10 5:27 PM ]

Picture and prose are fine. (One tweak: I would change "real problem" to merely "problem". Our choosing to solve it is the issue, not whether it is real).

Next steps: create this as a page in JIRA (which should be its home), and merge the content at http://clojure.org/patches (removing anything that is confusing or stale). Once the page is in JIRA, let Fogus know--he had some suggestions and edits.

When this is done ping me and I will change clojure.org/patches to link to JIRA.

Comment by Christopher Redinger [ 21/Jan/11 10:30 AM ]

Michael, can you review and edit with any feedback you have about the process? Thanks!

Comment by Stuart Sierra [ 21/Jan/11 12:22 PM ]

I was having trouble following the logic of the diagram, so with Aaron Bedra's help I drew my own:

https://docs.google.com/drawings/edit?id=1L4XyxO7ND7jgGvb69PD1-V41Rp4DfwjqiUiOcbsnus0&hl=en&authkey=COyn__kC

Comment by Christopher Redinger [ 21/Jan/11 1:08 PM ]

Thanks, I've updated with your document.

Comment by Christopher Redinger [ 10/Apr/11 12:05 AM ]

Doc is here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Christopher Redinger [ 15/Apr/11 12:03 PM ]

Please Screen





[CLJ-756] Workable upload destination for release zips Created: 11/Mar/11  Updated: 22/Apr/11  Resolved: 22/Apr/11

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Declined Votes: 0
Labels: None

Approval: Vetted

 Description   

We have been using Github's file upload under http://github.com/clojure for uploading releases

Pros:

  • looked easy
  • download tracking
  • tool we are already using

Cons:

  • unreliable
    • fails to upload
      • tested on multiple browsers with and without Flash
    • even when it works seems to show up during upload
      • presumably would fail the download in these cases
    • claims to upload but has garbage file
      • shows up in UI
      • permission error when you try to get it
      • sometimes causes browser to crash (not Github fault, but still...)
    • has added two hours of dev time to the current release (so far)

In order to get 1.3.0-alpha6 out the door (and end a multihour ordeal) I am pushing it to the files section at clojure.org.



 Comments   
Comment by Stuart Sierra [ 13/Mar/11 5:59 PM ]

What's behind clojure.org? Is it a real server we can log into or some kind of hosted app?

Comment by Christopher Redinger [ 22/Apr/11 9:34 AM ]

Will take this up with GitHub if we continue to have problems.





[CLJ-690] unchecked int math inconsistency with Java Created: 13/Dec/10  Updated: 08/Apr/11  Resolved: 05/Jan/11

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Handle-edge-case-in-arbitrary-precision-substraction.patch    
Patch: Code and Test
Approval: Ok

 Description   
Clojure 1.3.0-alpha4
user=> (-' 0 -9223372036854775808)
-9223372036854775808


 Comments   
Comment by Colin Jones [ 31/Dec/10 3:42 PM ]

This looks like a case where, in minusP, negateP was properly promoting the negation of -9223372036854775808 (Long/MIN_VALUE) to a BigInt, but that promotion didn't propagate to addP. The Ops that got used in addP was ops(-9223372036854775808), not ops(9223372036854775808N).

The attached patch makes sure the BigInt contamination goes all the way through, and adds a few tests verifying the correct behavior.





[CLJ-729] Add any-pred and every-pred combinators Created: 26/Jan/11  Updated: 20/Mar/11  Resolved: 20/Mar/11

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

Type: Enhancement Priority: Minor
Reporter: Fogus Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File 0729-add-every-any-pred.patch.diff     Text File 0729-with-consistent-names.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

Per the discussion at http://groups.google.com/group/clojure-dev/browse_frm/thread/899349c6a9b526e0

There is a general interest in a set of functions named any/every-pred that take a set of predicates and return a function that applies logical AND/OR to the application of each of its args. The naive implementations would be:

(defn every-pred [& preds] 
  (fn [& args] (every? #(every? % args) preds))) 

((every-pred pos? even?) 1 3 5 7 9)
;=> false

((every-pred pos? odd?) 1 3 5 7 9)
;=> true

(defn any-pred [& preds] 
  (fn [& args] (some #(some % args) preds)))

((any-pred pos? even?) -1 2 3 4 5 6)
;=> true

However, these implementations fall down in the following ways and should be corrected before inclusion to clojure.core:

1) The functions returned by each do not adhere to the same results as (and) and (or) given no arguments.

2) They and their returned functions assume varargs in every call. should be variadically unrolled in much the same way as juxt and comp. A longer term solution would be to implement them in terms of a macro that performs the unrolling automatically, but that is "extra-credit".

3) They should be tested in other_functions.clj



 Comments   
Comment by Fogus [ 26/Jan/11 11:27 AM ]

Added a patch containing the variadic unwound versions of every-pred and any-pred and tests for each.

Comment by Stuart Halloway [ 28/Jan/11 10:06 AM ]

One problem, and one question:

Problem: every-pred does not handler trueish arguments consistently.

((every-pred identity identity identity) 1 2)
=> 2

((every-pred identity identity identity identity) 1 2)
=> true

Since the name uses every, I think it should work like Clojure's every?, not like Clojure's and. Which raises the question about any-pred. If it is going to work like Clojure's some, shouldn't it be called some-pred?

I also found the docstrings confusing.

Comment by Fogus [ 28/Jan/11 11:13 AM ]

Problem: every-pred does not handler trueish arguments consistently.

Well, it consistently returns truthiness. I understand what you mean. There are at least two ways to handle this. Make every return filter through (boolean) or use every?/some in every case. Do you have a preference?

shouldn't it be called some-pred?

I buy that. Of course that might help to clarify my confusing docstrings (curse me for attempting an economy of verbiage).

Comment by Rich Hickey [ 28/Jan/11 3:48 PM ]

every-pred should return a boolean by filtering through boolean

any-pred should be some-fn

Comment by Fogus [ 08/Feb/11 9:05 AM ]

Updated patch.

Comment by Fogus [ 08/Feb/11 9:08 AM ]

Sorry it took me a while to get this in – I was a bit busy last week.

I modified the behavior of every/some-pred to return true or false and modified the tests to check this fact. Also simplified the docstrings. I could not delete the original patch, but since the attached files are sorted by date (and the new one is larger) it should be clear which is the valid patch file.

Comment by Stuart Halloway [ 22/Feb/11 8:24 PM ]

Third patch renames any-pred to some-fn to match Rich's naming suggestion.

Comment by Rich Hickey [ 25/Feb/11 9:49 AM ]

It looks like the patches and comments got crossed in the mail:

fogus' 2/8 patch correctly coerced to boolean in every-pred, but erroneously did so for some-pred.

screened patch did not incorporate either, but changed name to some-fn

We need:

every-pred - coerces to boolean
some-fn - doesn't

Thanks!

Comment by Fogus [ 25/Feb/11 7:11 PM ]

Let's try this again. All wires appear uncrossed.

Comment by Rich Hickey [ 01/Mar/11 2:11 PM ]

doc-only changes:

some-fn doc says it returns true, when it actually returns the first logical true value returned by one of the fns. Also, we don't yet use truthy/falsey in Clojure docs, and I'm not sure I want to start now.

Thanks

Comment by Fogus [ 02/Mar/11 7:50 AM ]

This has the docstring changes suggested by Rich (i.e. removing "truthy" "falsey" etc.).





[CLJ-432] deftype does not work if containing ns contains dashes Created: 08/Sep/10  Updated: 13/Mar/11  Resolved: 29/Nov/10

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

Type: Defect Priority: Major
Reporter: Laurent Petit Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJ-432.diff    
Patch: Code and Test
Approval: Ok

 Description   

There's a problem with the compilation (either live or AOT) of e.g. types, if the namespace containing the type definition cointains dashes.

It's quite easy to reproduce: create src/net/yournick/lr_plus.clj , and in this file, have just "(ns net.yournick.lr-plus) (deftype Foo)"
AOT compile this, and shebang! in your classes/ directory you see the following folder structure: classes/net/yournick/lr-plus/

lr-plus/ contains the Foo.class file

folder lr-plus/ should really be lr_plus/

the problem is that while apparently with Oracle JVMs everything works fine while you don't try to AOT compile it, it does not work (even if not AOT'ed) with IBM JRE 6:

Caused by: java.lang.ClassFormatError: JVMCFRE068 class name is invalid; class=compile__stub/net/cgrand/parsley/lr-plus/TableState, offset=0 (lr_plus.clj:8)



 Comments   
Comment by Assembla Importer [ 08/Oct/10 7:35 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/432

Comment by Chas Emerick [ 19/Nov/10 9:23 PM ]

This patch fixes namespace munging for protocols, deftype, and defrecord.

Comment by Laurent Petit [ 21/Nov/10 6:45 AM ]

OK, I've tested the patch and it works.
The correction seems straightforward, too.

Comment by Alex Miller [ 13/Mar/11 11:54 PM ]

This was a breaking change for us in 1.2.1 in this kind of example:

(ns my.rec-test
  (:use [clojure.test]))

(defrecord Foo [a])

(deftest test-rec 
  ;; 1.2.0 should use my.rec-test.Foo
  ;; 1.2.1 should use my.rec_test.Foo
  (is (= my.rec-test.Foo (class (Foo. 1)))))

Dropping a note here in case others run into it.





[CLJ-734] starting scope of let bindings seems incorrect from jdi perspective Created: 30/Jan/11  Updated: 11/Mar/11  Resolved: 11/Mar/11

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

Type: Defect Priority: Minor
Reporter: George Jahad Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-734.diff    
Approval: Ok

 Description   

It appears like the clojure compiler doesn't get the starting scope of let bindings right from the java debug interface perspective.

The compiler sets the starting scope of a local to the let/loop body, rather than to immediately after the local is bound. So when you are stepping through a let statement in a debugger, you can't eval the already defined bindings until you get to the let body.

Because it is a compiler problem, you see the symptoms with any jdi based debugger, (I've confirmed this on both jdb and cdt.)

The fix is pretty straightforward. Just gen.mark() a label after each binding is emitted and use that in the gen.visitLocalVariable() call instead of the loopLabel. I've attached a patch for clojure 1.2 branch. (If there is interest I'll create one for clojure master.)



 Comments   
Comment by George Jahad [ 30/Jan/11 7:15 PM ]

here is the diff against the current clojure master branch.

Comment by George Jahad [ 30/Jan/11 10:25 PM ]

re-added patch in proper format

Comment by George Jahad [ 31/Jan/11 12:41 PM ]

To test: start clojure with a jdi port, (like 8050 below)
java -agentlib:jdwp=transport=dt_socket,address=8050,server=y,suspend=n -cp /Users/georgejahad/incoming/clojure-1.3.0-alpha4/clojure.jar clojure.main

Then start jdb and attach to that port like so:
jdb -attach 8050

Set a breakpoint on pmap from jdb:
stop in clojure.core$pmap.invoke

Invoke pmap from the clojure repl:

(pmap inc (range 6))

From jdb, step over the first line of pmap, (that binds the local n):
next

From jdb, try to print the just bound local:
print n

Without the patch, you'll see:
com.sun.tools.example.debug.expr.ParseException: Name unknown: n
n = null

With it, you should see n equal to the correct value:
n = 4

Comment by Stuart Halloway [ 22/Feb/11 5:29 PM ]

Rich: does this need to be rewritten so that BindingInit is a value, or is the mutation of startScope ok?

Patch works against master.

Comment by Rich Hickey [ 25/Feb/11 9:40 AM ]

Yes, it is weird to mutate bindinginit with emit-related info. Better to keep a map of bindinginit->label inside doEmit, please.

Comment by George Jahad [ 26/Feb/11 12:39 PM ]

fixed to use HashMap of labels

Comment by Stuart Halloway [ 04/Mar/11 9:21 PM ]

Feb 26 patch is good





[CLJ-708] Multi-methods hold onto the head of their arguments Created: 08/Jan/11  Updated: 11/Mar/11  Resolved: 11/Mar/11

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: File muli-method-holds-head.diff     Text File multi-and-rest-fn-fix.patch     Text File multi-and-rest-fn-fix-unix.patch    
Patch: Code
Approval: Ok

 Description   

Multi-methods hold onto the head of their arguments when they are invoked. This means that if you invoke a multi-method with a lazy seq that cannot be held in memory all at once, you blow heap.

I'm not sure how best to write a test case for this particular issue, since the easiest way to test it is to run the JVM with a small heap and purposely evoke an OutOfMemoryError, so the attached patch has only the code changes. (However, I have verified the fix using a small heap.)

This will fix the issue for arities up to 6, for arities >=7 there is a bug in RestFn where it also holds the head of its arguments. If it is desirable to fix that bug as well, then I can submit a patch for it.



 Comments   
Comment by Paul Stadig [ 21/Jan/11 9:53 AM ]

Any thoughts on this?

Comment by Stuart Halloway [ 21/Jan/11 3:31 PM ]

I agree that no automated test is necessary for this. If you had attached your small-heap script I wouldn't have to write one from scratch to test it though.

Comment by Stuart Halloway [ 21/Jan/11 3:40 PM ]

Rich: two additional questions on this patch, other than just approval:

  1. Do we want another ticket to fix a similar issue in RestFn?
  2. Paul also inquired on the dev list about a 1.2.1 release for this fix. I dread the idea of going to point releases. Any suggestions on workarounds for this for people on the 1.2.x line?
Comment by Paul Stadig [ 21/Jan/11 4:17 PM ]

RE: a 1.2.1 release

This and the Keyword.intern fix both seem to be fixes for bugs that don't affect functionality, that would be worthy of a 1.2.1 release...I don't know if there are others that people would like to see in there.

They both also seem rather "trivial" to merge into 1.2. I'm willing to shepherd the release if no one else is interested in it.

Comment by Rich Hickey [ 26/Jan/11 6:54 AM ]

Patching both this and RestFn in a single go seems best.

Comment by Rich Hickey [ 26/Jan/11 6:55 AM ]

We should have a discussion about what might constitute a 1.2.1 on the dev list.

Comment by Paul Stadig [ 04/Feb/11 8:21 AM ]

Here is an updated patch for both multi-methods and RestFn.

I have also pushed a project to http://github.com/pjstadig/blow-heap that has some (gratuitous but automatically generated) tests for every arity combination.

There is a bin directory in that project that contains a blow-heap.sh file that can be run. It will call out to leiningen to run the tests, and the project.clj file is configured to start Java with a sufficiently small heap.

You can change the dependencies in the project.clj file to clojure 1.2.0, run the script, and see it fail. Then apply the patch to clojure, `mvn clean install`, go back to the blow-heap project and change the project.clj to use clojure 1.3.0 SNAPSHOT, `lein deps`, and rerun the tests to verify. (Whew!)

I believe this patch should also apply cleanly to the 1.2.0 branch, since the multi-method and rest-fn classes haven't changed since then, but if that's not the case I can send another rebased patch.

Comment by Stuart Halloway [ 22/Feb/11 7:58 PM ]

The second commit has the windows-line-ending problem. Anybody know how to fix this in an automated way?

Comment by Matthew Lee Hinman [ 22/Feb/11 11:45 PM ]

The same patch as Paul's, with unix line-endings

Comment by Matthew Lee Hinman [ 22/Feb/11 11:45 PM ]

Stuart: See attached patch run through the dos2unix tool

Comment by Stuart Halloway [ 06/Mar/11 1:18 PM ]

tests pass, approach looks fine. I didn't read every line, let mw know if there is some automated test or analysis you think I should do





[CLJ-749] reference a definterface in file that declares it Created: 02/Mar/11  Updated: 11/Mar/11  Resolved: 11/Mar/11

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix-0737-regression.patch    
Patch: Code and Test
Approval: Ok

 Description   

CLJ-737 introduced a regression: you can no longer refer to a definterface (e.g. as a type hint) later in the file that declares it. This is caused by the use of Java reflection, where the original code path used ASM reflection.

The patch will keep most of the enhancement of CLJ-737 (arrays of primitives), but will not support arrays of classes. (This did not trivially work, but can be made to work as a separate enhancement if anybody strongly needs it.)






[CLJ-741] Stop ISeq from inheriting Sequential Created: 19/Feb/11  Updated: 11/Mar/11  Resolved: 11/Mar/11

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

Type: Task Priority: Minor
Reporter: Chouser Assignee: Chouser
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Remove-Sequential-from-ISeq-s-implements-list-CLJ-74.patch     Text File 0741-iseq-sequential-resolved.patch    
Approval: Ok

 Description   

Currently, ISeq extends Sequential, which is what collections like vector use to determine equality partition membership. This prevents anything that implements ISeq from being exclusively in the map or set equality partition.

"Care will be required in making sure all appropriate concrete derivees remain Sequential. It's a breaking change in that some derivees not in Clojure may be relying on the derivation from ISeq"

http://groups.google.com/group/clojure-dev/browse_thread/thread/c58ec42d78209ee0



 Comments   
Comment by Chouser [ 19/Feb/11 2:00 AM ]

Here are the classes I found that implement ISeq and thus will need to be changed to implement Sequential:

  • LazySeq
  • ASeq
    • Range
    • EnumerationSeq
    • ArraySeq (including primitives)
    • IteratorSeq
    • Cons
    • StringSeq
    • ChunkedCons
    • PersistentList
    • PersistentVector Seq, RSeq and ChunkedSeq
    • PersistentMap KeySeq and ValSeq
    • PersistentHashMap Seq and NodeSeq
    • PersistentQueue Seq
    • PersistentTreeMap Seq
    • PersistentArrayMap Seq
    • PersistentStructMap Seq
  • IndexedSeq
    • ArraySeq (including primitives)
    • StringSeq
    • PersistentVector Seq and RSeq
  • IChunkedSeq
    • ChunkedCons
    • PersistentVector ChunkedSeq
    • VecSeq (from gvec)

Should ASeq, IndexedSeq, or IChunkedSeq implement Sequential in order to impart that to all their derived classes? My initial thought is that ASeq should reflect only ISeq and so shouldn't implement Sequential. But I find it hard to imagine indexed or chunked seqs that aren't sequential. Any thoughts?

Also, print-method currently prints all ISeqs with round parens – it seems to me this should be changed to test for Sequential instead. Does anyone disagree?

Comment by Rich Hickey [ 21/Feb/11 8:44 AM ]

> Should ASeq, IndexedSeq, or IChunkedSeq implement Sequential in order to impart that to all their derived classes?

Yes, all.

> Also, print-method currently prints all ISeqs with round parens – it seems to me this should be changed to test for Sequential instead. Does anyone disagree?

Sounds ok.

Comment by Chouser [ 04/Mar/11 11:55 AM ]

This patch does not change the print-method – it turns out that would have been a mistake.

Comment by Stuart Halloway [ 04/Mar/11 3:40 PM ]

My patch is same as Chouser's except tweaked to make the git gods happy.





[CLJ-747] stack overflow diffing large objects Created: 28/Feb/11  Updated: 02/Mar/11  Resolved: 02/Mar/11

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0747-diffing-large-associatves.patch    
Patch: Code
Approval: Ok

 Description   

e.g.

(clojure.data/diff (range 50000) (range 50000))))





[CLJ-748] fast path equal case for diff Created: 28/Feb/11  Updated: 02/Mar/11  Resolved: 02/Mar/11

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0748-fast-equal-diff.patch    
Patch: Code
Approval: Ok

 Description   

For the use case of clojure.data/diff in comparing test results and expectations, it would be nice to do minimal work if the objects are equal. A simple check is low-hanging fruit.






[CLJ-737] definterface/gen-interface do not support array parameter and return types Created: 13/Feb/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Major
Reporter: Daniel Solano Gómez Assignee: Daniel Solano Gómez
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File definterface-array-fix-with-tests.patch    
Patch: Code
Approval: Ok

 Description   

As describe in this clojure-dev post, gen-interface and definterface do not properly support array type hints.

Ultimately, this is because gen-class and gen-interface use two different code paths to do essentially the same thing. Boiled down, gen-class converts hints using (Type/getType (the-class c)), whereas gen-interface uses asm-type, which uses similar, but different, logic than the-class.

In my patch, I change asm-type to match use the-class. I also add entries to the prim->class map to support primitive arrays.



 Comments   
Comment by Daniel Solano Gómez [ 24/Feb/11 12:40 PM ]

An updated patch that includes tests for the new functionality.





[CLJ-380] bit-and missing long parameters overload Created: 14/Jun/10  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Critical
Reporter: David Powell Assignee: David Powell
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-add-missing-overloads-for-numerics-to-prevent-major-.patch     Text File 0380-numeric-overloads-2.patch    
Patch: Code
Approval: Ok

 Description   

[Updated: The new primitive support relies on a primitive bit-and operator. As one doesn't exist, clojure functions like even? are now using reflective calls and are about 150x slower than in Clojure 1.2)

JVM has a land instruction and java has & operator that works with longs.

public static long and(long, long);
Code:
0: lload_0
1: lload_2
2: land
3: lreturn

The attached patch allows Clojure to take advantage of it.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:41 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/380
Attachments:
0001-Added-bit-and-with-long-parameters-and-overload-reso.patch - https://www.assembla.com/spaces/clojure/documents/dciMoWD8ir36WfeJe5cbLA/download/dciMoWD8ir36WfeJe5cbLA

Comment by Assembla Importer [ 24/Aug/10 11:41 AM ]

digash said: Just found that Rich is already taking care of this case in the num branch:

http://github.com/richhickey/clojure/commit/c5d0985af6c17103eaabe523e442f14c29916266#L3R1510

The test in the patch could still be useful.

Comment by Assembla Importer [ 24/Aug/10 11:41 AM ]

digash said: Is it possible to include this in 1.2? Since the num branch is not going to be included in 1.2.

Comment by David Powell [ 24/Jan/11 5:21 PM ]

The primitives work means that bit-and, when used with numeric literals, attempts to call the primitive overloads in the Numbers class.
As these overloads don't exist, the code makes a reflective + boxed call. This is extremely slow compared to Clojure 1.2.

Clojure 1.2.0-master-SNAPSHOT
user=> (time (count (filter even? (range 1e6))))
"Elapsed time: 248.719099 msecs"

Clojure 1.3.0-alpha4
user=> (time (count (filter even? (range 1e6))))
"Elapsed time: 46679.133155 msecs"

See http://groups.google.com/group/clojure-dev/browse_thread/thread/a1f0f03e11d90f59

Comment by David Powell [ 24/Jan/11 5:27 PM ]

Attached is a patch which adds missing numeric overloads to:

and
andNot
or
xor
divide

Comment by Stuart Halloway [ 22/Feb/11 6:21 PM ]

Second patch removes unnecessary overloads of andNot. Performance tested good using code below. At some point it would be good to get documentation on why some fns are inline and others are static.

(def test-data (range 1 1e4))

(def fn-sources
  '[(fn [a b] (bit-and a b))
    (fn [a b] (bit-and (long a) b))
    (fn [a b] (bit-and a (long b)))
    (fn [a b] (bit-and (long a) (long b)))
    (fn [a b] (bit-and-not a b))
    (fn [a b] (bit-and-not (long a) b))
    (fn [a b] (bit-and-not a (long b)))
    (fn [a b] (bit-and-not (long a) (long b)))
    (fn [a b] (bit-or a b))
    (fn [a b] (bit-or (long a) b))
    (fn [a b] (bit-or a (long b)))
    (fn [a b] (bit-or (long a) (long b)))
    (fn [a b] (bit-xor a b))
    (fn [a b] (bit-xor (long a) b))
    (fn [a b] (bit-xor a (long b)))
    (fn [a b] (bit-xor (long a) (long b)))
    (fn [a b] (/ a b))
    (fn [a b] (/ (long a) b))
    (fn [a b] (/ a (long b)))
    (fn [a b] (/ (long a) (long b)))])

(defn time-numeric-ops
  [fn-sources]
  (doseq [source fn-sources]
    (print source ": ")
    (let [f (eval source)]
      (time
       (do
         (doall (map f test-data test-data))
         nil)))))

(time-numeric-ops fn-sources)




[CLJ-702] case gives NPE when used with nil Created: 04/Jan/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Major
Reporter: Benjamin Teuber Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: GZip Archive 0702-fix-npe-in-nil-case-2.patch.gz     GZip Archive 0702-fix-npe-in-nil-case-3.patch.gz     Text File 0702-fix-npe-in-nil-case.patch    
Patch: Code and Test
Approval: Ok

 Description   

This code gives a NullPointerException:

(case nil
nil "foo")

If fixing this is impossible for implementation reasons, at least the documentation and error messages should be improved.



 Comments   
Comment by Aaron Bedra [ 05/Jan/11 9:28 PM ]

What would be the desired fix look like for this? Should this be a documentation update or should this be updated to not produce an NPE?

Comment by a_strange_guy [ 06/Jan/11 4:23 PM ]

I think that the case macro should just generate a null check before calling hashcode on a value. The same way protocol functions automatically check for null before dispatching.

Comment by Rich Hickey [ 07/Jan/11 8:03 AM ]

First step in fixing a problem is to understand it. The exception is thrown during compilation and the stack trace points to c.l.Compiler$ObjExpression.constantType()

Comment by Aaron Bedra [ 07/Jan/11 9:31 AM ]

Thanks Rich. I will take this one and get a patch in hopefully today.

Comment by Aaron Bedra [ 07/Jan/11 2:17 PM ]

Rich, this seems to solve the issue demonstrated in this ticket. I don't have full context on all possible impact points here. Can you please take a look at this and let me know if I missed anything?

Comment by Rich Hickey [ 10/Jan/11 6:35 AM ]

looks ok at first glance

Comment by Stuart Sierra [ 21/Jan/11 1:44 PM ]

New patch 0702-fix-npe-in-nil-case-2.patch.gz adds a test.

Comment by Aaron Bedra [ 25/Jan/11 7:48 AM ]

Stuart, your patch replaces the original patch and the commit message doesn't reveal the intent of the fix. It also overwrites me as the original patch submitter.

Comment by Stuart Sierra [ 25/Jan/11 2:50 PM ]

Oops. Sorry. Will fix.

Comment by Stuart Halloway [ 28/Jan/11 10:23 AM ]

waiting on Stuart's patch tweak

Comment by Stuart Sierra [ 28/Jan/11 3:00 PM ]

Patch 0702-fix-npe-in-nil-case-3.patch.gz replaces previous patches, but preserves correct commit messages and authors.





[CLJ-715] pprint test failing only on Hudson Created: 14/Jan/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0715-eliminate-fragile-test.patch    
Patch: Code and Test
Approval: Ok

 Description   

The commit https://github.com/clojure/clojure/commit/5a86d525f10f32304f2cb348c9383934a44e6d4b comments out a pprint of agents test that fails only on Hudson. May be a race condition.

You can see the Hudson console log at http://build.clojure.org/job/clojure/236/console (no login required).






[CLJ-728] Test for error message fails in IBM JDK Created: 25/Jan/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

IBM JDK on Hudson, http://build.clojure.org/job/ibm-jdk-clojure-build/


Attachments: GZip Archive 0728-reify-exceptions-1.patch.gz    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

A test for reify looks for an error message matching a specific regex, which fails on IBM JDK:

FAIL in (reify-test) (
you can't define a method twice
expected: (fails-with-cause? java.lang.ClassFormatError #"^(Repetitive|Duplicate) method name" (eval (quote (reify java.util.List (size [_] 10) java.util.Collection (size [_] 20)))))
actual: #<CompilerException java.lang.ClassFormatError: (clojure/test_clojure$eval17188$reify__17189) duplicate method at offset=1052, compiling/var/lib/hudson/jobs/ibm-jdk-clojure-build/workspace/src/script/run_tests.clj:257)>



 Comments   
Comment by Stuart Sierra [ 28/Jan/11 3:14 PM ]

Patch 0728-reify-exceptions-1.patch.gz changes the test to not check for specific types or messages on the exceptions.

Comment by Stuart Sierra [ 11/Feb/11 1:43 PM ]

This is the only thing holding us back from a successful build on IBM JDK.





[CLJ-739] version.properties file is not closed Created: 15/Feb/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: GZip Archive clj-739-a.patch.gz    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

Originally reported by Olek at https://groups.google.com/d/topic/clojure/jhdSyljImuU/discussion

When loading core.clj, the stream opened on the version.properties file is never closed. This can cause problems in some environments such as JEE.



 Comments   
Comment by Stuart Sierra [ 18/Feb/11 9:28 AM ]

Fix in clj-739-a.patch

Comment by Rich Hickey [ 25/Feb/11 8:05 AM ]

Please don't zip your patches, thanks!

Comment by Stuart Sierra [ 25/Feb/11 8:34 AM ]

Got it.





[CLJ-742] Error message for invalid map literals is not helpful Created: 22/Feb/11  Updated: 25/Feb/11  Resolved: 25/Feb/11

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

Type: Defect Priority: Trivial
Reporter: Luke VanderHart Assignee: Luke VanderHart
Resolution: Completed Votes: 0
Labels: None

Attachments: File 742.diff    
Patch: Code
Approval: Ok

 Description   

Literal maps with an odd number of forms give an IndexOutOfBounds exception, which is unhelpful and misleading in determining the source of the error. They should instead return a more specific error, something like "map literal must contain an even number of forms".



 Comments   
Comment by Luke VanderHart [ 23/Feb/11 7:43 PM ]

Patch submitted.





[CLJ-280] def should support an optional doc-string Created: 11/Mar/10  Updated: 28/Jan/11  Resolved: 12/Oct/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

To be more consistent across all "operators that introduce names", def should support an optional docstring after the name.



 Comments   
Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/280
Attachments:
0001-add-docstring-support-to-def.patch - https://www.assembla.com/spaces/clojure/documents/cwJXMqr-Kr35PieJe5aVNr/download/cwJXMqr-Kr35PieJe5aVNr
0280-def-docstring-resolved.patch - https://www.assembla.com/spaces/clojure/documents/bOM0ly1umr36tseJe5cbLA/download/bOM0ly1umr36tseJe5cbLA

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

bsteuber said: [file:cwJXMqr-Kr35PieJe5aVNr]

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

bsteuber said: Contributor Agreement coming soon..

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

bsteuber said: Now it arrived

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

stu said: [file:bOM0ly1umr36tseJe5cbLA]

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

stu said: Second patch subsumes first and adds some minor cleanup.

Comment by Assembla Importer [ 12/Oct/10 6:58 PM ]

stu said: Updating tickets (#276, #280, #378, #437, #448)

Comment by Benjamin Teuber [ 22/Jan/11 9:50 AM ]

In my initial patch, I forgot to update clojure.repl to have (doc def) mention the optional docstring. I've written a patch for this small doc update, but it seems I can't upload it anymore in a closed issue. Should I just mail it to someone?

Comment by Stuart Halloway [ 28/Jan/11 9:07 AM ]

I made this change before seeing your comment.

Comment by Benjamin Teuber [ 28/Jan/11 3:02 PM ]

I see - funny coincedence
My patch had the additional sentence

"An optional documentation string can be supplied (only when init is given, too)."

at the end of the documentation to be clear about what happens when only one of init and doc is given, but I'm not sure this really is necessary.

Loved your book by the way





[CLJ-8] GC Issue 3: Re-enable detection of circular loads Created: 17/Jun/09  Updated: 28/Jan/11  Resolved: 28/Jan/11

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

Type: Enhancement Priority: Minor
Reporter: Stephen C. Gilardi Assignee: Stephen C. Gilardi
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File 0008-cyclic-deps-resolved.patch    
Patch: Code and Test
Approval: Ok

 Description   
Reported by richhickey, Dec 17, 2008
Was made a no-op for AOT/gen-class - need to distinguish cases

Comment 1 by richhickey, Dec 17, 2008
(No comment was entered for this change.)
Owner: ---
Comment 2 by richhickey, Dec 18, 2008
(No comment was entered for this change.)
Labels: Priority-High
Comment 3 by scgilardi, May 12, 2009
Rich, do you have an example of the problem with AOT/gen-class that motivated
disabling detection of circular loads? With that in hand, I'd like to take a look at
fixing this.
Comment 4 by richhickey, May 13, 2009
You should just try it out. I'm not sure if it is just something I encountered on my
way to the final mechanism (and now isn't needed). However, you should hold off right
at the moment, as I'm working on loading/classloader stuff.


 Comments   
Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/8
Attachments:
load-tracking.diff - https://www.assembla.com/spaces/clojure/documents/arZXq-2qKr3R8teJe5aVNr/download/arZXq-2qKr3R8teJe5aVNr
check-cyclic-load.diff - https://www.assembla.com/spaces/clojure/documents/aUykkoZfSr35wNeJe5cbCb/download/aUykkoZfSr35wNeJe5cbCb

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

scgilardi said: I'm working on this now. I plan to have more info by 19 October.

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

scgilardi said: [file:arZXq-2qKr3R8teJe5aVNr]: fixes tickets 8, 42, and 113

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

scgilardi said: please see my posting to the developer group

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

scgilardi said: [file:aUykkoZfSr35wNeJe5cbCb]: patch to detect/reject cyclic deps

Comment by Assembla Importer [ 30/Sep/10 2:42 PM ]

scgilardi said: fresh patch that detects and rejects cyclic dependencies. based on today's master. tests included.

Comment by Stephen C. Gilardi [ 07/Jan/11 1:31 PM ]

I provided a patch for this. Is there anything further I can or must do to to move it along?

Comment by Stuart Halloway [ 15/Jan/11 3:50 PM ]

Had to recreate the patch almost from scratch. Haven't seen this problem before--when the git merge failed, it wouldn't create any .rej files to work with.

Recreated patch is same as Steve's, except for a test configuration change to match current MASTER.





[CLJ-719] clojure.data/diff does not correctly handle arrays as first argument Created: 17/Jan/11  Updated: 28/Jan/11  Resolved: 28/Jan/11

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

Type: Defect Priority: Minor
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0719-diff-for-arrays.patch    
Patch: Code and Test
Approval: Ok

 Description   

The EqualityPartition implementation correctly branches, placing arrays into the sequential partition. The Diff implementation needs a similar branch.



 Comments   
Comment by Stuart Halloway [ 17/Jan/11 4:42 PM ]

This patch is smaller than it looks: the only real change is the one line in Object's diff-similar. The rest is just extracting diff-sequential as a helper (now called from two places), and grouping the private helpers together in the file.





[CLJ-710] clojure/set fns don't work with mutable sets Created: 09/Jan/11  Updated: 14/Jan/11  Resolved: 14/Jan/11

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0710-diff-mutable.patch    
Patch: Code and Test
Approval: Ok

 Description   

You can't call union, intersection, difference, et al on a mix of Clojure sets and Java sets, because internally the fns assume that sets are clojure values that you can call conj, disj, etc. on.

This percolates up and causes problems for clojure.data/diff as well, since diff aspires to work for any collection, but uses clojure.set to deal with sets.

Possible solutions:

  1. clojure.set fns detect mutable sets and convert to values at the outset
  2. clojure.set doesn't do magic conversions for you. But diff, which is less perf-sensitive, can do so. diff makes immutable values out of mutable sets before handing off to clojure.set
  3. Let the caller beware: docstring in clojure.set tells you to use immutable sets.


 Comments   
Comment by Stuart Halloway [ 09/Jan/11 10:31 AM ]

Rich, do you like option #1 in the description, or option #2 + #3, or none of the above?





[CLJ-684] agent self-send test heisenfailing Created: 30/Nov/10  Updated: 19/Dec/10  Resolved: 19/Dec/10

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Alexander Redington
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0684-fix-race-condition.patch    
Approval: Ok

 Description   

The commented-out test (https://github.com/clojure/clojure/commit/605944b7f667e9fdcc2a380c5dd07304118dca34) is failing intermittently. If you are working on this, look at the changes made in CLJ-672, which may be related.



 Comments   
Comment by Alexander Redington [ 17/Dec/10 1:43 PM ]

My initial research today suggests this might more likely be an issue with await-for than agent error handlers. After examining a couple of different variants of the offending test, I've found that:

  • The test won't fail without await-for returning due to timing out
  • The test won't fail if we substitute await for await-for
Comment by Stuart Halloway [ 17/Dec/10 4:01 PM ]

Alex: you didn't try hard enough to reproduce – you can make it happen even with await.

But you diagnosis got me on the right track to fixing this. Thanks! Patch in a moment.

Comment by Stuart Halloway [ 17/Dec/10 4:05 PM ]

The original tests had a race condition: you can't use await to wait on agent actions triggered from another thread. Improved version of the tests uses a CountdownLatch.





[CLJ-31] GC Issue 27: Disallow recur across try Created: 17/Jun/09  Updated: 18/Dec/10  Resolved: 17/Dec/10

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

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Kevin Downey
Resolution: Completed Votes: 0
Labels: None

Attachments: File 31-recur-across-try3.diff    
Approval: Ok

 Description   
Reported by richhickey, Jan 01, 2009
Right now will allow:

(loop [x 42]
  (when (pos? x)
    (binding [*out* *out*]
      (recur (dec x)))))

but the recur doesn't (can't) really happen in the scope of the binding


 Comments   
Comment by Assembla Importer [ 26/Oct/10 12:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/31
Attachments:
31-recur-across-try.diff - https://www.assembla.com/spaces/clojure/documents/aeZrIo4kWr352reJe5cbCb/download/aeZrIo4kWr352reJe5cbCb
31-recur-across-try2.diff - https://www.assembla.com/spaces/clojure/documents/cffLE64syr369QeJe5cbLA/download/cffLE64syr369QeJe5cbLA

Comment by Assembla Importer [ 26/Oct/10 12:30 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 26/Oct/10 12:30 AM ]

hiredman said: [file:aeZrIo4kWr352reJe5cbCb]: patch to disallow recur across try

Comment by Assembla Importer [ 26/Oct/10 12:30 AM ]

hiredman said: [file:cffLE64syr369QeJe5cbLA]: try #2, patch to disallow recur across try

Comment by Assembla Importer [ 26/Oct/10 12:30 AM ]

hiredman said: did a try one, realized it didn't work and my tests were wrong, try 2 seems to actually work

Comment by Stuart Halloway [ 30/Oct/10 11:29 AM ]

Patch does two (related) things:

  • throws compile-time exception if you recur across try
  • throws compile-time exception if you recur across dynamic binding

Rich: is this the desired behavior?

Comment by Rich Hickey [ 30/Oct/10 2:44 PM ]

It would be nice if the tests confirmed ok things still work, e.g. a full loop/recur inside a try etc

Comment by Kevin Downey [ 02/Nov/10 3:23 PM ]

well, dynamic bindings are a try/finally pushing and popping the bindings

Comment by Kevin Downey [ 19/Nov/10 9:46 PM ]

try #3 at the patch, now with some tests to check that things that are ok to do are still possible





[CLJ-678] into-array should work with all primitive types Created: 21/Nov/10  Updated: 17/Dec/10  Resolved: 17/Dec/10

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 678.patch    
Approval: Ok

 Description   

e.g.

(into-array Integer/TYPE [0])
(into-array Byte/TYPE [0])

which currently fail with an argument type mismatch.



 Comments   
Comment by Stuart Halloway [ 17/Dec/10 9:15 AM ]

Good task for pairing with Luke. Feel free to grab me and discuss approach.

Comment by Stuart Halloway [ 17/Dec/10 3:10 PM ]

Side question while working on this ticket: Should I create a separate ticket so that {{(into-array [1 2 3]}} returns an array of long instead of Long, or it that too magical?

Comment by Rich Hickey [ 17/Dec/10 3:15 PM ]

Until [1 2 3] is a vector of longs, that's too magical.





[CLJ-300] newline should output platform-specific newline sequence Created: 17/Apr/10  Updated: 17/Dec/10  Resolved: 17/Dec/10

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0300-newline-local-var-6-commits.patch     Text File 0300-newline-resolved.patch     Text File 0300-newline-with-pprint-and-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

(newline), and (println) currently output \n as the newline sequence, even on Windows, where \r\n is the newline sequence.
These functions should output the platform specific newline sequence.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/300
Attachments:
0001-fix-newline-to-use-newline-sequence-appropriate-for-.patch - https://www.assembla.com/spaces/clojure/documents/agy8tGsJqr35CCeJe5aVNr/download/agy8tGsJqr35CCeJe5aVNr
0300-newline-platform-default.patch - https://www.assembla.com/spaces/clojure/documents/bn-GzURhKr34gXeJe5cbCb/download/bn-GzURhKr34gXeJe5cbCb

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

rnewman said: It might be nice, then, to introduce 'newln' and 'newline', with the former doing platform-specific line breaks, and the latter printing the newline character.

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

djpowell said: If you want an explicit line feed character, then you're probably best just printing \newline explicitly. In Java out.newline(), and out.println() always use platform specific newlines. Also, in C, printf("\n") is defined to output the platform specific newline to text streams.

println and friends operate on characters rather than bytes, so I think it makes sense for you to get newline translation in addition to character encoding.

Also the patch attached allows an alternative line-separator to be dynamically bound, should you need to write files for other platforms.

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

stu said: This seems reasonable, but I don't live on Windows. Can you add a pointer to list or IRC discussion and re-set this ticket to test?

In particular I would like to see discussion of (1) whether any existing code might depend on the current behavior and (2) dynamic binding vs. explicit argument to bypass platform behavior.

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

djpowell said: Started discussion here:
http://groups.google.com/group/clojure/browse_thread/thread/70f2945110f209e1#

I don't think there is much point in an explicit parameter to newline - if you want to be that specific, you may as well use (print "\r\n")?

But it is probably reasonable to want non-platform newlines occasionally by dynamic binding, eg to interact with swank (which prefers \n), or HTTP and MIME which prefer \r\n.

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

stu said: 1. Agree that a specific override parameter for newline is bad – if you want to do something specific, just do it.

2. But by the same token, I would rather not introduce a new bindable var. I would just change newline to always print the result of (System/getProperty "line.separator"). Not totally sure about this, though. What do others think?

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

stu said: [file:bn-GzURhKr34gXeJe5cbCb]

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

stu said: Second patch wins, per unanimous agreement on dev list at http://groups.google.com/group/clojure-dev/browse_thread/thread/4f1812a1f96a3347.

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

richhickey said: I don't think we should be looking up a system property on every call to newline

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

richhickey said: Also, what is the relationship between (newline) and \newline ?

Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

djpowell said: Re: calling a system property on every call

  • thinking back, that is probably one of the reasons that I made this a dynamic var in the original version of my patch. I agree, it doesn't seem elegant to keep checking a system property. We might be better using a dynamic var, even if we just keep the var private for now if there is any concern about not wanting to support rebindng. I assume that a dynamic var is faster than a system property?

Re: the relationship between newline and \newline

  • (newline) outputs the platform dependent newline sequence. It works like System.out.println() in Java, or Console.WriteLine() in C#. Note that (println) and similar clojure functions call newline to output line endings.
  • \newline is simply the constant value of the ASCII 0x0a linefeed character. This is the same semantics as "\n" in Java and C#. Note that this is different to stdio in C where "\n" has different semantics depending on whether the stream that it is written to was opened in text or binary mode.
Comment by Assembla Importer [ 24/Aug/10 10:33 AM ]

richhickey said: We can cache the property without a dynamic var and all that that implies

Comment by Stuart Halloway [ 29/Oct/10 1:13 PM ]

After sleeping on it (in a hammock), I still think that respecting the Java default is the way to go. If you want something different, it is easy enough to do:

  • if you want only a single newline, print \newline
  • if you want something else, then there is no precedent for it in Java or Clojure, so roll-your-own.

The most recent patch (0300-newline-resolved) does this, but breaks some tests of cl_format. Soliciting feeback from Tom Faulhaber before pushing this to screened.

Comment by Tom Faulhaber [ 29/Oct/10 4:59 PM ]

From my email to Stu:

I think that cl-format should emit platform specific newlines (this is consistent with the recommendation in CLtL2, section 2.2.2) when confronted the ~% directive. I suspect that that's where the tests are failing. CLtL2 also specifies that \newline will do the same thing, but I assume that we're not going there.

I'll try to get you an extended patch with improved tests over the weekend.

Comment by Tom Faulhaber [ 04/Nov/10 11:39 PM ]

This patch includes pprint/cl-format in the change and fixes all the tests so they can run on either Unix or Windows.It replaces (and includes/corrects) previous patches.

Comment by Tom Faulhaber [ 04/Nov/10 11:42 PM ]

OK, this should be ready to go.

Comment by David Powell [ 25/Nov/10 10:13 AM ]

I think it would be worth caching the value of the system property in a private def. System.getProperties invokes synchronisation, security managers, and other heavy-weight things.

Comment by Stuart Halloway [ 29/Nov/10 8:42 AM ]

Two questions about avoiding the system property lookup:

  1. Should a cached value be a public def, a private def, or just a closure?
  2. The JDK implementation caches in a private field, but they use a privileged action and a sun-specific class to request the property! Can we just do a simple getProperty?
Comment by Rich Hickey [ 03/Dec/10 12:00 PM ]

1) private def
2) Yes

Comment by Stuart Halloway [ 17/Dec/10 10:02 AM ]

Dec 17 patch addresses all comments, and includes cleanup of test helpers (coalesce to top of test hieararchy so that committers can find them)





[CLJ-465] with-local-vars broken after changes to make dynamic off by default Created: 23/Oct/10  Updated: 12/Dec/10  Resolved: 12/Dec/10

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

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

with-local-vars should create dynamic vars. Here is what happens now in HEAD

user=> (defn factorial [x]
(with-local-vars [acc 1, cnt x]
(while (> @cnt 0)
(var-set acc (* @acc @cnt))
(var-set cnt (dec @cnt)))
@acc))
#'user/factorial
user=> (factorial 5)
IllegalStateException Can't dynamically bind non-dynamic var: null/null clojure.lang.Var.pushThreadBindings (Var.java:352)

Attached is a patch to mark with-local-vars vars as dynamic, and a test to prove that it works using factorial .



 Comments   
Comment by Assembla Importer [ 25/Oct/10 2:02 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/465
Attachments:
with-local-vars-dynamic.patch - https://www.assembla.com/spaces/clojure/documents/ca7m5W3Sar34MEeJe5cbLA/download/ca7m5W3Sar34MEeJe5cbLA

Comment by Stuart Halloway [ 12/Dec/10 8:37 PM ]

patch was already applied but ticket never updated





[CLJ-390] sends from agent error-handlers should be allowed Created: 29/Jun/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Chouser
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0003-Allows-agent-error-handler-to-send.patch    
Patch: Code and Test
Approval: Ok

 Description   

Currently if an agent's error-handler sends to any agent, the sent action is
silently thrown away. The send should instead be allowed.

Originally reported here:
http://groups.google.com/group/clojure/browse_frm/thread/c1d05bdbb50d7d28



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/390
Attachments:
0001-Allows-agent-error-handler-to-send-successfully.-Ref.patch - https://www.assembla.com/spaces/clojure/documents/d9EEI0G4yr36YneJe5cbLr/download/d9EEI0G4yr36YneJe5cbLr
0390-agent-error-send-plus-test.patch - https://www.assembla.com/spaces/clojure/documents/cVNaMc15Wr35VzeJe5cbLr/download/cVNaMc15Wr35VzeJe5cbLr

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

chouser@n01se.net said: [file:d9EEI0G4yr36YneJe5cbLr]: Set nested to null instead of to empty vector before invoking error handler

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

richhickey said: This needs careful analysis

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

chouser@n01se.net said: When the agent's thread-local "nested" field is non-nil, any sends are queued instead of being sent immediately. This is its normal state when agent actions are being executed.

Another piece of thread-local state is agent which is set to the current agent when an action is being executed. This state is also checked by 'await' so that it can throw if it's called during an agent action.

I can think of two options for allowing sends from agent error handlers:

A. Set "nested" to null before the error handler is invoked, allowing any sends from it to proceed immediately when send is called.

B. Clear "nested" to an empty vector as it is now, but then after invoking the error handler release any newly pending sends queued by the error handler.

This patch implements choice (A). It changes the state of the agent's "nested" field from an empty vector to null for the period of time between when the action has terminated with an error and the time when "nested" is normally set to null (after the next action, if any, has been queued. The things that happen during this time are:

1. the error handler (if any) is invoked
2. the error flag is reset (if error-mode is :continue)
3. the current action is popped off the agent's queue
4. the next action is queued

So this change allows sends from this agent during all of these steps. It does not change agent, so 'await' is still disallowed throughout these steps.

Of the 4 things done during this time period, only step 1 calls any user code in the current thread such that thread-local values would have any impact, and none of the others depend on the value of "nested" at all.

During step 1, the user's error handler will be called with agent true and nested nil, which is combination not normally seen. The reasons that sends from inside actions are normally held until later is prevent them from happening if the action fails to "commit" and update the agent's value, and to promise to users that any nested actions will see the updated agent's value (or a subsequent value). Neither of these reasons apply in this case because the state of the agent will not be updated due to the failed action, so allowing sends to proceed immediately is safe.

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

shoover said: For what it's worth, I confirm that this patch works for the situations I've tested. My desired case of having an error handler send to a supervisor agent works great.

I tried to break it by having the handler fn send to agent and await agent, but it all goes pretty much like you described.

;; Sending to *agent* from a handler works fine. Note the outer deref will not
;; always see the value :handled because the action that returned :handled
;; was sent from a different thread (the action thread).
(let [handler (fn [_ ex]
                (send *agent* (fn [_] :handled)))
      a (agent 42 :error-handler handler)]
  (send a (fn [_] (throw (Exception. "bad news"))))
  (await a)
  @a)

await agent in a handler throws an exception as expected, and it is silently eaten like any other handler exception.

The weirdest case is if you mess up in the handler and send agent an action that awaits agent : you get an infinite loop through the handler (if error mode is :continue). I wouldn't recommend sending to agent in a handler, of course, but it's possible with this patch in the same way it's possible to add a watch that performs another send to the same agent. It'll go all day.

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

stu said: [file:cVNaMc15Wr35VzeJe5cbLr]

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

stu said: I agree with Chouser's reasoning (in the comments).

Comment by Stuart Halloway [ 05/Nov/10 11:06 AM ]

Heisenbugs! Test no longer passing, don't have time to look at it atm. Moving back from OK to Test.

Comment by Chouser [ 07/Nov/10 5:29 PM ]

Test appears to failing because of CLJ-672

Comment by Chouser [ 08/Nov/10 12:54 AM ]

The test added previously was failing because *agent* wasn't bound in the error handler, per CLJ-672.

This patch (0003-) subsumes the previous patches and adds a similar test that does not rely on *agent* being bound. This would have succeeded all along. After a fix for CLJ-672, both tests pass.

Setting this back to Test, but it will fail unless tested after applying the patch from CLJ-672.





[CLJ-674] Typo and/or out-of-place example in clojure.string docstring. Created: 09/Nov/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

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

Type: Defect Priority: Major
Reporter: Phil Hagelberg Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-674_clojure.str-doc-fix.diff    
Patch: Code
Approval: Ok

 Description   

Clojure String utilities

It is poor form to (:use clojure.string). Instead, use require
with :as to specify a prefix, e.g.

(ns your.namespace.here
(:require '[clojure.string :as str]))

-----------

The quote in the :require clause is erroneous. On the other hand, clojure.string's ns docstring may not be the place for an explanation of use-vs-require, so the whole paragraph could be removed.



 Comments   
Comment by Fogus [ 23/Nov/10 7:20 PM ]

Added patch.

Comment by Fogus [ 23/Nov/10 7:23 PM ]

This is a trivial fix for a docstring that is clearly incorrect. Thanks Phil.





[CLJ-320] alias function gives confusing message if using unknown namespace Created: 28/Apr/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

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

Type: Defect Priority: Minor
Reporter: Aaron Bedra Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Attachments: File 0320-alias-error-message-clarification.diff     File 320_davidrupp.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Aaron Bedra

 Description   

This is really a request for improved error handling. If you use (alias) with a namespace that is not known it throws an NPE with imho a confusing error message.

Example:

user=> (alias 'R 'S) 
java.lang.NullPointerException: Expecting Symbol + Namespace (NO_SOURCE_FILE:0)
user=> (.printStackTrace *e)
nil
user=> java.lang.NullPointerException: Expecting Symbol + Namespace (NO_SOURCE_FILE:0)
        at clojure.lang.Compiler.eval(Compiler.java:5348)
        at clojure.lang.Compiler.eval(Compiler.java:5300)
        at clojure.core$eval__4281.invoke(core.clj:2135)
        at clojure.main$repl__6358$read_eval_print__6366.invoke(main.clj:183)
        at clojure.main$repl__6358.doInvoke(main.clj:200)
        at clojure.lang.RestFn.invoke(RestFn.java:422)
        at clojure.main$repl_opt__6392.invoke(main.clj:254)
        at clojure.main$main__6418.doInvoke(main.clj:347)
        at clojure.lang.RestFn.invoke(RestFn.java:398)
        at clojure.lang.Var.invoke(Var.java:361)
        at clojure.lang.AFn.applyToHelper(AFn.java:161)
        at clojure.lang.Var.applyTo(Var.java:482)
        at clojure.main.main(main.java:37)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at jline.ConsoleRunner.main(ConsoleRunner.java:69)
Caused by: java.lang.NullPointerException: Expecting Symbol + Namespace
        at clojure.lang.Namespace.addAlias(Namespace.java:184)
        at clojure.core$alias__4566.invoke(core.clj:2892)
        at user$eval__11.invoke(NO_SOURCE_FILE:7)
        at clojure.lang.Compiler.eval(Compiler.java:5332)
        ... 17 more
</code></pre>

And the clj:

<pre><code>(defn alias
  "Add an alias in the current namespace to another
  namespace. Arguments are two symbols: the alias to be used, and
  the symbolic name of the target namespace. Use :as in the ns macro in preference
  to calling this directly."
  [alias namespace-sym]
  (.addAlias *ns* alias (find-ns namespace-sym)))

Clearly, the clojure is doing find-ns and the namespace is not known so it passes a null down. Seems like the code in either the clj or java level could say "Hey dummy, you used an unknown namespace" instead of "Expecting Symbol + Namespace". Maybe this would be useful as an additional sentence in the alias doc string too.



 Comments   
Comment by Assembla Importer [ 26/Oct/10 11:07 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/320

Comment by Assembla Importer [ 26/Oct/10 11:07 PM ]

aaron said: Tried to submit a patch but keep getting 500 errors on assembla. I'll try again later, but i'll paste the patch in here so it is somewhere else.

From 32c32d65ec43598f36872a742bdebec5907d17d7 Mon Sep 17 00:00:00 2001
From: Aaron Bedra <aaron@aaronbedra.com>
Date: Tue, 26 Oct 2010 23:34:55 -0400
Subject: [PATCH] clarifying error message on alias


src/jvm/clojure/lang/Namespace.java | 9 +++++++--
1 files changed, 7 insertions, 2 deletions

diff --git a/src/jvm/clojure/lang/Namespace.java b/src/jvm/clojure/lang/Namespace.java
index 1936973..87a9082 100644
— a/src/jvm/clojure/lang/Namespace.java
+++ b/src/jvm/clojure/lang/Namespace.java
@@ -210,8 +210,13 @@ public Namespace lookupAlias(Symbol alias){
}

public void addAlias(Symbol alias, Namespace ns){

  • if (alias == null || ns == null)
  • throw new NullPointerException("Expecting Symbol + Namespace");
    + if (alias == null && ns == null)
    + throw new NullPointerException("Received unknown alias and namespace");
    + else if (ns == null)
    + throw new NullPointerException("Received unknown namespace");
    + else if (alias == null)
    + throw new NullPointerException("Received unknown alias");
    +
    IPersistentMap map = getAliases();
    while(!map.containsKey(alias))
    {

    • 1.7.3.2
Comment by Stuart Halloway [ 29/Oct/10 3:20 PM ]

If we are going to the trouble of improving the error message, then we should have the error message refer to the arg that caused the problem.

By handling the error in the internal method addAlias instead of the API fn alias, you lose the value passed in, and cannot say "You said foo, but foo isn't a namespace."

Patch that adds error handling to alias in core.clj would be welcome.

Comment by David Rupp [ 30/Oct/10 3:43 PM ]

I had some private correspondence with Aaron about this attempt. As he rightly says, it does handle only the case where the aliased namespace is nil, not when the alias itself is nil, but I think that is by far the most likely scenario. On the plus side, it does so in core.clj using existing API. Maybe a good starting point?

Comment by Aaron Bedra [ 12/Nov/10 1:15 PM ]

After another review of David's patch I think it is the way to go here. Please use his patch.





[CLJ-454] docstrings for special ops Created: 08/Oct/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Chouser
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Make-doc-support-docstrings-for-special-ops.-CLJ-454.patch     Text File 0001-Move-doc-and-find-doc-to-repl-support-docstrings-for.patch    
Patch: Code
Approval: Ok

 Description   

"If someone wants to submit a patch with doc support for the special ops, I'll take it, as long as they contain links to the full docs and aren't too long themselves" – rhickey

From here: http://clojure-log.n01se.net/date/2010-10-08.html#10:10



 Comments   
Comment by Assembla Importer [ 10/Oct/10 2:42 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/454

Comment by Chouser [ 10/Nov/10 8:54 PM ]

Should doc, find-doc, print-doc, etc. be moved out of clojure.core into clojure.repl as part of this?

Comment by Chouser [ 13/Nov/10 9:57 PM ]

This patch updates doc to show docstrings for special forms and more useful URLs for Java interop special forms. Special forms that are not fronted by macros have docs in a private var clojure.core/special-doc-map which doc and find-doc use. This patch also has find-doc search namespace docstrings.

Comment by Chouser [ 13/Nov/10 10:23 PM ]

This patch is essentially the same as the other, but also moves doc, find-doc, and related code to clojure.repl. This is technically a breaking change, but since doc and find-doc are also added to clojure.main's list of vars to refer, anyone using a clojure.main's repl won't notice any difference.

Comment by Colin Jones [ 27/Nov/10 9:55 PM ]

I love this change. Having special form docs in the REPL will be really helpful for newbies.

A couple questions/comments on the second patch:

1. Moving it to clojure.repl makes a lot of sense semantically. I wouldn't guess many people use doc in their actual production code, but I'm guessing this would require updates to tools like autodoc.

2. In clojure.repl/doc, why use #'print-doc and #'special-doc rather than just the bare symbols for those functions?

Comment by Tom Faulhaber [ 28/Nov/10 12:36 AM ]

This is great. When these patches are applied, I'll add this stuff to the autodoc.





[CLJ-462] clojure.core/slurp docstring out of date Created: 19/Oct/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

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

Type: Defect Priority: Trivial
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Completed Votes: 0
Labels: None

Attachments: Java Source File 0001-Updated-the-docstring-on-slurp-to-indicate-it-accept.patch    
Patch: Code
Approval: Ok

 Description   

The slurp function used to only work on filename arguments, but now it uses clojure.java.io/reader instead, so it works with a much wider variety of arguments. The docstring does not reflect this change.

Something like this might work: "Opens a reader on f and reads all its contents, returning a string. See clojure.java.io/reader for a complete list of supported arguments."



 Comments   
Comment by Assembla Importer [ 19/Oct/10 5:36 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/462

Comment by Alexander Redington [ 12/Nov/10 12:23 PM ]

Update for using the suggested docstring.





[CLJ-441] Add unchecked coercions Created: 28/Sep/10  Updated: 08/Dec/10  Resolved: 26/Nov/10

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

Type: Enhancement Priority: Blocker
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0441-unchecked-coercions.patch    
Patch: Code and Test
Approval: Ok

 Description   

Checking in coercions has been added, making them safe, but removing the bit-twiddling power. We need some unchecked (truncating) coercion ops



 Comments   
Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/441
Attachments:
unchecked-casts.diff - https://www.assembla.com/spaces/clojure/documents/dVK0q-Znmr37hgeJe5cbLA/download/dVK0q-Znmr37hgeJe5cbLA
unchecked-casts-without-typo.diff - https://www.assembla.com/spaces/clojure/documents/alGr58Znur34SreJe5cbCb/download/alGr58Znur34SreJe5cbCb
0441-unchecked-coercions-plus-table.patch - https://www.assembla.com/spaces/clojure/documents/c1gyXG1xKr37lweJe5cbLr/download/c1gyXG1xKr37lweJe5cbLr

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

richhickey said: See also #284 which will be addressed by this

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

ataggart said: [file:dVK0q-Znmr37hgeJe5cbLA]: implementation and tests for unchecked casts

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

ataggart said: Added unchecked casts from every numeric primitive and Object to every numeric primitive. Character and char can only get cast to int, as per the convention with the checked cast.

Added tests for the above in clojure.test-clojure.numbers.

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

ataggart said: [file:alGr58Znur34SreJe5cbCb]: Same as above but without the typo in the docs

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

stu said: [file:c1gyXG1xKr37lweJe5cbLr]

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

stu said: Third patch subsumes previous two and adds a test table for various conversions.

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

richhickey said: I'd be interested in better names than unchecked-*

any ideas? Think also about unchecked-add et al

Comment by Assembla Importer [ 30/Sep/10 11:42 PM ]

richhickey said: let's create a clojure.unchecked ns and put these and the other unchecked arithmetic ops in there, using same names as normal, e.g. , inc etc. Only put the long versions in there, we'll have to decide separately about the truncating int-add etc. Then people can either use unchecked or alias and do uc/ etc

Comment by Rich Hickey [ 29/Oct/10 6:58 AM ]

Let's create a clojure.unchecked ns and put these and the other unchecked arithmetic ops in there, using same names as normal, e.g. +, inc etc. Only put the -long versions in there, we'll have to decide separately about the truncating int-add etc. Then people can either use unchecked or alias and do uc/+ etc

Comment by Rich Hickey [ 25/Nov/10 8:50 AM ]

error: patch failed: test/clojure/test_clojure/numbers.clj:31

Comment by Aaron Bedra [ 26/Nov/10 12:44 PM ]

Fixed up the patch to merge properly as of 8225407032ea643cbe3db7f35ef97b1230fc65b8





[CLJ-680] printing promises should not block Created: 26/Nov/10  Updated: 08/Dec/10  Resolved: 08/Dec/10

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0680_nonblocking_promise_printing.patch    
Patch: Code
Approval: Ok

 Description   

Do not tackle the bigger "abstractions around visibility, delay, blocking" etc. issue. This is a minimal fix:

  • print code already deals with future as a special case
  • should special-case promise as well
  • make a definterface IPromiseImpl that you can switch on
    • right above promise in core.clj
    • has a method that gets you what you need to ask "has-value?"
  • in print code
    • if has value, show it, if not, show :not-delivered
  • make sure to fix print code in both core_print and pprint





[CLJ-292] LazySeq.sval() nests RuntimeExceptions Created: 08/Apr/10  Updated: 08/Dec/10  Resolved: 08/Dec/10

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

Type: Enhancement Priority: Minor
Reporter: Daniel Solano Gómez Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

When evaluating a LazySeq, if an exception occurs it gets wrapped in a RuntimeException and is thrown. Unfortunately, the logic does not differentiate between a RuntimeException and a checked exception. This has what I believe are two unintended side effects.

  1. If a function throws a RuntimeException, such as an IllegalArgumentException, it gets wrapped in a RuntimeException unnecessarily. So, if I am expecting an IllegalArgumentException (as in a test), I will unexpectedly get a RuntimeException instead.
  2. When evaluating nested lazy sequences, if an error occurs, the root exception is wrapped over and over again as the exception winds its way up the stack. In a recent project, this meant having extremely long stack traces that include a half dozen nested RuntimeExceptions. These just get in the way of getting to the root cause.

Both of these scenarios are illustrated in the attached file.

My proposed solution is fairly simple: Instead of indiscriminately wrapping every exceptions a RuntimeException, simply rethrow any RuntimeException. As a result, any RuntimeException will simply wind its way back up the stack with no wrapping, and checked exceptions will only be wrapped once. This should make debugging Clojure programs slightly easier as the resulting stack trace will not include as much irrelevant information.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 4:56 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/292
Attachments:
wrapped_exception.clj - https://www.assembla.com/spaces/clojure/documents/d7u-xuqYir34eneJe5avMc/download/d7u-xuqYir34eneJe5avMc
fix-292.diff - https://www.assembla.com/spaces/clojure/documents/blThXoqYur35a4eJe5afGb/download/blThXoqYur35a4eJe5afGb

Comment by Assembla Importer [ 28/Sep/10 4:56 PM ]

dsg said: [file:blThXoqYur35a4eJe5afGb]: Proposed fix





[CLJ-199] chunked seqs support 'for' byte code can't be converted to dex for dalvik Created: 18/Oct/09  Updated: 15/Nov/10  Resolved: 01/Nov/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Aaron Bedra
Resolution: Declined Votes: 0
Labels: None


 Description   

Commit 306ef6 generates bytecode which can not be covered to dex for dalvik. See following git-bisect output for details.

306ef6d9e47253103ca0e5ae0f4b620d5fa2aeff is first bad commit
commit 306ef6d9e47253103ca0e5ae0f4b620d5fa2aeff (14316ae2110a779ffc8ac9c3da3f1c41852c4289)
Author: Chouser <chouser@n01se.net>
Date:   Sun Aug 23 21:36:41 2009 -0400

    Add support for chunked seqs to 'for'.  Refs #1

:040000 040000 d48a1b12db408d4bc5843367ea5529678522f57d 7d8717380716feab9ebc84883e60dceea8fb1e08 M src
:040000 040000 0d80fce85701005c13ce13c105524a4fff188396 5daece53b0dd2727dc7ffa9c0b773e7962de900d M test
bisect run success

processing clojure/core$generate_proxy__5911$iter__5947__5953$fn__5954$iter__5949__5956$fn__5957.class...

UNEXPECTED TOP-LEVEL EXCEPTION:
com.android.dx.cf.code.SimException: local variable type mismatch: attempt to set or access a value of type java.lang.Object using a local variable of type int. This is symptomatic of .class transformation tools that ignore local variable information.
 at com.android.dx.cf.code.BaseMachine.throwLocalMismatch(BaseMachine.java:537)
 at com.android.dx.cf.code.Simulator$SimVisitor.visitLocal(Simulator.java:504)
 at com.android.dx.cf.code.BytecodeArray.parseInstruction(BytecodeArray.java:472)
 at com.android.dx.cf.code.Simulator.simulate(Simulator.java:96)
 at com.android.dx.cf.code.Ropper.processBlock(Ropper.java:681)
 at com.android.dx.cf.code.Ropper.doit(Ropper.java:636)
 at com.android.dx.cf.code.Ropper.convert(Ropper.java:253)
 at com.android.dx.dex.cf.CfTranslator.processMethods(CfTranslator.java:252)
 at com.android.dx.dex.cf.CfTranslator.translate0(CfTranslator.java:131)
 at com.android.dx.dex.cf.CfTranslator.translate(CfTranslator.java:85)
 at com.android.dx.command.dexer.Main.processClass(Main.java:297)
 at com.android.dx.command.dexer.Main.processFileBytes(Main.java:276)
 at com.android.dx.command.dexer.Main.access$100(Main.java:56)
 at com.android.dx.command.dexer.Main$1.processFileBytes(Main.java:228)
 at com.android.dx.cf.direct.ClassPathOpener.processArchive(ClassPathOpener.java:245)
 at com.android.dx.cf.direct.ClassPathOpener.processOne(ClassPathOpener.java:130)
 at com.android.dx.cf.direct.ClassPathOpener.process(ClassPathOpener.java:108)
 at com.android.dx.command.dexer.Main.processOne(Main.java:245)
 at com.android.dx.command.dexer.Main.processAllFiles(Main.java:183)
 at com.android.dx.command.dexer.Main.run(Main.java:139)
 at com.android.dx.command.dexer.Main.main(Main.java:120)
 at com.android.dx.command.Main.main(Main.java:87)
...at bytecode offset 000000d7
locals[0000]: Lclojure/core$generate_proxy__5911$iter__5947__5953$fn__5954$iter__5949__5956$fn__5957;
locals[0001]: known-null
locals[0002]: known-null
locals[0003]: known-null
locals[0004]: known-null
locals[0005]: I
locals[0006]: Ljava/lang/Object;
stack[0003]: Lclojure/lang/IFn;
stack[0002]: Ljava/lang/Object;
stack[0001]: Ljava/lang/Object;
stack[top0]: known-null
...while working on block 00cd
...while working on method invoke:()Ljava/lang/Object;
...while processing invoke ()Ljava/lang/Object;
...while processing clojure/core$generate_proxy__5911$iter__5947__5953$fn__5954$iter__5949__5956$fn__5957.class


 Comments   
Comment by Assembla Importer [ 25/Oct/10 11:11 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/199

Comment by Assembla Importer [ 25/Oct/10 11:11 PM ]

richhickey said: what are the tools/commands needed to reproduce this?

Comment by Assembla Importer [ 25/Oct/10 11:11 PM ]

remvee said: Oeps, the SHA1 in this report is not part of the official git repo but
only in my tree. I've digged around and found it at 14316a.

Anyway, the problem does not occur in the current master branch but
persist in 1.1.x branch. So please close this issue.

Just for reference, you can reproduce the problem as follows:

  • download the android SDK
  • build clojure.jar
  • run: [andriod_sdk]/platforms/android-2.0.1/tools/dx --dex clojure.jar
  • boom
Comment by Assembla Importer [ 25/Oct/10 11:11 PM ]

aaron said: The current master 09ba677c1dfd8e75cec47d270062558c71aa551d does not cause this issue. I tested on android-7 and android-8 platforms with fresh downloads of everything. Should we close this ticket?

Comment by Aaron Bedra [ 29/Oct/10 9:44 AM ]

Contacted original poster about closing this issue.

Comment by Aaron Bedra [ 01/Nov/10 7:38 AM ]

Orignal ticket creator agreed that this is no longer an issue. This issue was fixed sometime between 1.1 and 1.2 but since there is no patches or details on what changed here I am marking the resolution declined





[CLJ-336] top-level nav for clojure.org Created: 04/May/10  Updated: 15/Nov/10  Resolved: 01/Nov/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Approval: Accepted

 Comments   
Comment by Assembla Importer [ 24/Aug/10 7:52 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/336

Comment by Stuart Halloway [ 01/Nov/10 8:43 AM ]

Alex Miller is helping out with clojure.org edits and has added items that were missing from the left nav.





[CLJ-410] clojure.xml parse/emit do not round-trip Created: 23/Jul/10  Updated: 15/Nov/10  Resolved: 30/Oct/10

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

Type: Defect
Reporter: Anonymous Assignee: Rich Hickey
Resolution: Declined Votes: 0
Labels: None


 Description   

I'm not expecting (emit (parse x)) to produce the same characters as originally in x. (That would be canonical form.)
However, it seems reasonable to expect that (= (parse x) (parse (emit (parse x)))). That's not currently the case because of #277 and #408.
It's not currently possible to demonstrate this bug via a unit test because of #409.
This fails:

(deftest parse-and-emit-should-round-trip-data
  (are [s0]
       (let [x1 (str->xml s0)
             s2 (xml->str x1)
             x3 (str->xml s2)]
         (is (= x1 x3)))
       "<e/>"
       "<e></e>"
       "<e>content</e>"
       "<e a='attribute'/>"
       "<e a='attribute'>content</e>"
       "<e><e><e/></e></e>"
       "<e> </e>"
       "<e>&lt;&amp;&gt;</e>"))
</code></pre>
Where:
<pre><code>(defn xml->str [x]
  (with-out-str
    (xml/emit-element x)))

(defn str->xml [s]
  (xml/parse
   (ByteArrayInputStream. (.getBytes s "UTF-8"))))

Or, rather it crashes prematurely in str->xml because of #409.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 11:19 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/410
Attachments:
410-emit-element-no-longer-emits-spurious-new.patch - https://www.assembla.com/spaces/clojure/documents/b6SULoLZer346_eJe5cbLr/download/b6SULoLZer346_eJe5cbLr

Comment by Assembla Importer [ 28/Sep/10 11:19 AM ]

bpsm said: Tests that demonstrate #410 are available at http://github.com/bpsm/test-clojure-xml.

Comment by Assembla Importer [ 28/Sep/10 11:19 AM ]

bpsm said: [file:b6SULoLZer346_eJe5cbLr]: see also http://github.com/bpsm/clojure/commits/fix408,410,277

Comment by Assembla Importer [ 28/Sep/10 11:19 AM ]

stu said: Duplicated association with ticket #277 was added

Comment by Assembla Importer [ 28/Sep/10 11:19 AM ]

chouser@n01se.net said: Please consider the solution provided by clojure.contrib.lazy-xml/emit which uses javax.xml.transform classes to fix not only these bugs (#410 and #408) but also support indent, xml-declaration, and encoding options.

Comment by Stuart Halloway [ 30/Oct/10 2:46 PM ]

emit is not part of Clojure's public API, and we don't want to grow a public API via an issue-driven random walk. If you are interested in this issue, please chime in on the design page for a new data.xml library: http://dev.clojure.org/display/DXML/Home





[CLJ-442] Fix embedded doc links on clojure.org docs Created: 28/Sep/10  Updated: 15/Nov/10  Resolved: 30/Oct/10

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

Type: Defect
Reporter: Anonymous Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

These broke when we moved to clojure org on github. Try WebDAV for bulk edit without having to troll through the pages manually



 Comments   
Comment by Assembla Importer [ 28/Sep/10 8:06 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/442

Comment by Stuart Halloway [ 30/Oct/10 11:18 AM ]

completed by Alex Miller. Thanks!





[CLJ-448] structural diff Created: 30/Sep/10  Updated: 05/Nov/10  Resolved: 12/Oct/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

I constantly find myself writing ad hoc code to compare bits of structure, either at the REPL or in tests. (Or even worse, trying to eyeball two long pprint outputs).

What I want instead is a diff function that compares two structures a and b, returning the parts only in a, parts only in b, and parts in both. Good solution for my needs should:

  • work with all kinds of collections (both Clojure and Java)
  • make simple choices about how to define a "difference"
  • need not worry too much about perf


 Comments   
Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/448
Attachments:
0448-structural-diff.patch - https://www.assembla.com/spaces/clojure/documents/dr3uxCZj4r36h-eJe5cbLr/download/dr3uxCZj4r36h-eJe5cbLr
0448-structural-diff-mm.patch - https://www.assembla.com/spaces/clojure/documents/dXJg-aZlyr35yieJe5cbCb/download/dXJg-aZlyr35yieJe5cbCb
0448-structural-diff-no-mms.patch - https://www.assembla.com/spaces/clojure/documents/bkIb2GZnOr363HeJe5cbLr/download/bkIb2GZnOr363HeJe5cbLr
0448-data-diff.patch - https://www.assembla.com/spaces/clojure/documents/dhs7kcZ88r34I5eJe5cbCb/download/dhs7kcZ88r34I5eJe5cbCb
0448-data-diff-privatized.patch - https://www.assembla.com/spaces/clojure/documents/b7-JX21Hur36gceJe5cbCb/download/b7-JX21Hur36gceJe5cbCb
0448-data-diff-nil-atom.patch - https://www.assembla.com/spaces/clojure/documents/aoWwgo1Iqr36gceJe5cbCb/download/aoWwgo1Iqr36gceJe5cbCb

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:dr3uxCZj4r36h-eJe5cbLr]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Not attached to the namespace or fn names, nor to having this in Clojure as opposed to contrib. Just had to put it somewhere to solicit feedback.

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

laurentpetit said: Did you know about difform before starting this ? Looks like an interesting implementation :

http://github.com/GeorgeJahad/difform

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:dXJg-aZlyr35yieJe5cbCb]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Hmm, difform is cool, but coming at the problem from a different direction. I think there is room for both.

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Second patch (using a multimethod) reads better I think.

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:bkIb2GZnOr363HeJe5cbLr]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: third patch (no mms) closes off potential for accidental comparison of unlikes

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

richhickey said: wow, reading patches of patches of patches is difficult

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

richhickey said: is the partitioning the same as for equality? If so it could be split out

the j.u.Collection ==> sequence case makes me wary - should it be j.u.List?

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:dhs7kcZ88r34I5eJe5cbCb]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: fourth patch (0448-data-diff.patch) is hopefully final:

  • replaces the others in a single clean patch.
  • partitions by List instead of Collection
  • renames the protocol and its fns to (better?) names
Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

richhickey said: is the partitioning the same as for equality? If so it could be split out

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:b7-JX21Hur36gceJe5cbCb]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: 0448-data-diff-privatized.patch subsumes the others. Changes:

  • separated a protocol for EqualityPartition
  • marked things as private / implementation detail
  • used correct namespace clojure.data
  • added copyright notice
Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: [file:aoWwgo1Iqr36gceJe5cbCb]

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: 0448-data-diff-nil-atom subsumes others, and puts nil into the :atom EqualityPartition

Comment by Assembla Importer [ 12/Oct/10 7:58 PM ]

stu said: Updating tickets (#276, #280, #378, #437, #448)

Comment by Bryan Weber [ 05/Nov/10 3:02 PM ]

Does this diff include or ignore metadata? I frequently want to compare structures in unit tests, but I would actually like to take the metadata into account... It would be great to have as an option.





[CLJ-397] better error message when calling macros with arity Created: 07/Jul/10  Updated: 05/Nov/10  Resolved: 05/Nov/10

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

Type: Enhancement Priority: Blocker
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0397-arity-unwrapped.patch    
Patch: Code and Test
Approval: Ok

 Description   

The two magic arguments to macros lead to weird error messages:

(defmacro foo [a b] a)
(foo)
java.lang.IllegalArgumentException: Wrong number of args (2) passed to: user$foo (NO_SOURCE_FILE:)0


 Comments   
Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/397
Attachments:
mh-397-arity.patch - https://www.assembla.com/spaces/clojure/documents/atduak20Cr35rOeJe5cbLA/download/atduak20Cr35rOeJe5cbLA

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

stu said: Updating tickets (#427, #426, #421, #420, #397)

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

stu said: The choke point here is line 5286 of Compiler.java. At this point, we know we are in macro, but we don't know we if we have an arity problem. When the arity problem is throw, it is a plain old IllegalArgumentException. Some choices:

(1) Hack: parse the exception, pull out the number, subtract 2. Yuck.

(2) Make the exception smarter (subclass the exception as ArityException or some such, catch that here, subtract 2.

(3) Make AFunction smarter, so that it knows if it is a macro or not (also requires change to defmacro). Then throwArity can do the right thing at the point of the problem.

Option #2 feels smaller than #3, but maybe knowing when an AFunction is AMacro will be useful elsewhere.

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

richhickey said: #2 please

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

fogus said: I agree with #2 also, but in general I think in creating a (reasonable) set of exception subclasses we could start down the path of making error reporting and handling more friendly.

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

stu said: Updating tickets (#429, #437, #397, #420)

Comment by Assembla Importer [ 18/Oct/10 11:07 PM ]

mikehinchey said: [file:atduak20Cr35rOeJe5cbLA]: fixes using Stuart's #2 choice

Comment by Stuart Halloway [ 29/Oct/10 10:26 AM ]

Second patch (arity-unwrapped) subsumes first, and does not return a wrapped exception. Wrapped exception would defeat the purpose of the patch, by giving REPL root-cause consumers an incorrect message.





[CLJ-447] biginteger fn misses a case for clojure.lang.BigInt Created: 29/Sep/10  Updated: 05/Nov/10  Resolved: 05/Nov/10

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

Type: Defect Priority: Blocker
Reporter: Aaron Bedra Assignee: Aaron Bedra
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

(biginteger 13178456923875639284562345789N)
==> #<IllegalArgumentException java.lang.IllegalArgumentException: Value out of range for long: 13178456923875639284562345789>



 Comments   
Comment by Assembla Importer [ 25/Oct/10 7:25 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/447
Attachments:
fixbug447.diff - https://www.assembla.com/spaces/clojure/documents/dzCDwS2B0r3543eJe5cbLA/download/dzCDwS2B0r3543eJe5cbLA

Comment by Assembla Importer [ 25/Oct/10 7:25 PM ]

ataggart said: [file:dzCDwS2B0r3543eJe5cbLA]: adds conversion and test

Comment by Assembla Importer [ 25/Oct/10 7:25 PM ]

aaron said: reviewed and tested against 09ba677c1dfd8e75cec47d270062558c71aa551d





[CLJ-458] print-table Created: 14/Oct/10  Updated: 05/Nov/10  Resolved: 05/Nov/10

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

Type: Enhancement Priority: Blocker
Reporter: Assembla Importer Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

Print a textual table from a collection of maps. Useful for viewing regular data at the REPL.



 Comments   
Comment by Assembla Importer [ 15/Oct/10 10:29 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/458
Attachments:
0458-pr-table.patch - https://www.assembla.com/spaces/clojure/documents/dVt9Xg18Sr34Z7eJe5cbCb/download/dVt9Xg18Sr34Z7eJe5cbCb
0458-print-table.patch - https://www.assembla.com/spaces/clojure/documents/b5Fqn62gGr349VeJe5cbLr/download/b5Fqn62gGr349VeJe5cbLr

Comment by Assembla Importer [ 15/Oct/10 10:29 AM ]

stu said: [file:dVt9Xg18Sr34Z7eJe5cbCb]

Comment by Assembla Importer [ 15/Oct/10 10:29 AM ]

richhickey said: I just realized it should be called print-table if not readable, sorry I didn't think of this before

Comment by Assembla Importer [ 15/Oct/10 10:29 AM ]

stu said: [file:b5Fqn62gGr349VeJe5cbLr]

Comment by Assembla Importer [ 15/Oct/10 10:29 AM ]

stu said: second patch uses correct name





[CLJ-460] promote interrupt handling to clojure.repl Created: 15/Oct/10  Updated: 05/Nov/10  Resolved: 05/Nov/10

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

Type: Enhancement Priority: Blocker
Reporter: Chouser Assignee: Chouser
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Add-set-break-handler-and-thread-stopper-refs-CLJ-46.patch     Text File 0002-Add-set-break-handler-and-thread-stopper-CLJ-460.patch    
Patch: Code
Approval: Ok

 Description   

The functions start-handling-break and add-break-thread! in clojure.contrib.repl-utils are regularly needed by tools such as the Clojure Debugging Toolkit. I propose we promote them to clojure.repl, eliminating what for many projects is their sole dependency on old contrib.

Issues to consider before OKing:

  • the usage of sun.misc classes appears to be the only game in town. For comparison, JRuby and Groovy already offer access to these classes, Scala appears not to.
  • the :need-init code in start-handling-break is too cute, but ain't broke, so won't change it.
  • do we need to add code to clean up the weak references? If so:
    • call each time a thread is added?
    • switch to ConcurrentHashMap to match code already in Clojure?


 Comments   
Comment by Assembla Importer [ 25/Oct/10 10:05 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/460

Comment by Assembla Importer [ 25/Oct/10 10:05 PM ]

richhickey said: Is there a patch to look at?

Comment by Assembla Importer [ 25/Oct/10 10:05 PM ]

chouser@n01se.net said: 'add-break-thread!' is too compound, presumably to make it easier to give instructions in IRC. No reason it couldn't be simplified, making the instructions to set up two steps, one to register a thread to break, another to start catching the Ctrl-C. I'll prepare a patch and docs for this.

Comment by Chouser [ 27/Oct/10 10:32 PM ]

This patch avoids the issues of cleaning up weak references by only directly supporting one thread to stop at a time. I think this is a sensible compromise as you'll generally only have one main REPL thread at a time. In a more complex situation, stopping all registered threads isn't obviously the right thing to do. set-break-handler! accepts a function that can do arbitrarily complex things as needed.

This patch also avoids the cuteness of :need-init by simply relying on the calling process to only call set-break-handler! once, which any REPL-startup script should be able to easily comply with. It appears that repeated calls to set-break-handler! will just overwrite the old handler with the new one – if their the same (such as the default handler) then no harm done. If they're not the same, refusing to overwrite the old one is not obviously better than just taking the new handler.

Comment by Rich Hickey [ 28/Oct/10 8:09 AM ]

I think the default should throw a derivee of Error, not Exception, as there are many Exception-eating paths in arbitrary code.

Comment by Chouser [ 28/Oct/10 8:26 AM ]

Ah, figured out how to set to "Test". I assume this is ok to do, even though I'm not "Core team"?

Comment by Chouser [ 28/Oct/10 8:28 AM ]

Bah, sorry, missed Rich's comment. Back to Approval: None. Will fix the patch.

Comment by Chouser [ 28/Oct/10 10:05 PM ]

The two reasonable choices of existing Errors to throw are Error and ThreadDeath. ThreadDeath is the default for thread .stop, but doesn't provide a mechanism for setting the message. I think it's more important to specify the exact cause of the exception, that a SIGINT was caught, than to use a possibly more accurate exception type, so the new patch throws a plain Error.

Comment by george jahad [ 29/Oct/10 12:15 PM ]

Looks good to me.

Comment by Chouser [ 29/Oct/10 12:25 PM ]

I just realized that since this will be in clojure.repl now, clojure.main could do (set-break-handler!) in repl-opt so that plain terminal REPLs would catch Ctrl-C by default. Would this be desirable? Hm, perhaps that warrants a separate issue.

Comment by Stuart Halloway [ 29/Oct/10 1:26 PM ]

Second patch is good. Let's consider repl-opt feature separately.





[CLJ-235] clojure.test fixtures don't work with test-ns-hook Created: 01/Jan/10  Updated: 05/Nov/10  Resolved: 05/Nov/10

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

I just noticed something about clojure.test that isn't documented, and might not be by design.

The docs say:

"Fixtures allow you to run code before and after tests, to set up
the context in which tests should be run."

and also

"By default, these functions will search for all tests defined in
a namespace and run them in an undefined order. However, if you
are composing tests, as in the "arithmetic" example above, you
probably do not want the "addition" and "subtraction" tests run
separately. In that case, you must define a special function
named "test-ns-hook" that runs your tests in the correct order:

(defn test-ns-hook []
     (arithmetic))
"

The actual behavior is that fixtures are run if you don't specify your own test-ns-hook</code>. You can figure this out if you read the source, or from the docstring of <code>test-ns if you know how fixtures are implemented, but it's certainly not clear.

Might I suggest that one of the following be done?

  • The docs are extended or clarified to warn that fixtures will not be run if you define your own test-ns-hook, or
  • test-ns</code> be extended (and <code>test-all-vars</code> refactored) to call fixtures prior to calling the <code>test-ns-hook, or
  • An exported function be introduced (and test-all-vars refactored to use it) to thread execution via the ns fixtures, so that hooks can use it?

I'm leaning towards the second or third option; the third allows the test-ns-hook</code> to optionally invoke the fixtures, which adds flexibility, but of course if you're defining a <code>test-ns-hook you're at liberty to simply not define the fixtures at al