<< Back to previous view

[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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-665] Add mechanism to temporarily replace root bindings of vars Created: 29/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: Minor
Reporter: Chouser Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Add-with-redefs-macro-and-with-redefs-fn-CLJ-665.patch     Text File 0665-redefs-better-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

There are cases, such as mocking out functions during testing, where it is useful to replace the root bindings of vars (dynamic or otherwise) with new values temporarily, and have them reliably set back to their original values when done.

Some discussion here: http://clojure-log.n01se.net/date/2010-10-29.html#09:11



 Comments   
Comment by Chouser [ 29/Oct/10 10:30 AM ]

Attached patch includes a with-redefs macro and a with-redefs-fn, as well as a couple tests.

Comment by Stuart Halloway [ 01/Nov/10 8:40 AM ]

Second patch subsumes first, improves tests





[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-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-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-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-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-453] Reflection Created: 07/Oct/10  Updated: 15/Oct/10  Resolved: 15/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   

Clojure API for reflection that

(1) separates API from engine, so that an ASM-based reflector can be used without having to load classes
(2) provides a REPL interface that is a successor to clojure.contrib.repl-utils/show
(3) returns data, not encapsulated objects



 Comments   
Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/453
Attachments:
0453-reflection.patch - https://www.assembla.com/spaces/clojure/documents/biy_oS0Jir36FceJe5cbLA/download/biy_oS0Jir36FceJe5cbLA
0453-reflection-matching-wip.patch - https://www.assembla.com/spaces/clojure/documents/ci50PA1ZCr36bDeJe5cbCb/download/ci50PA1ZCr36bDeJe5cbCb
0453-clojure-reflect.patch - https://www.assembla.com/spaces/clojure/documents/bcLZTE19mr36vzeJe5cbCb/download/bcLZTE19mr36vzeJe5cbCb

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: [file:biy_oS0Jir36FceJe5cbLA]

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: Note that the tests depend on #448.

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

richhickey said: I created http://dev.clojure.org/display/design/Reflection+API for discussion and notes

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: [file:ci50PA1ZCr36bDeJe5cbCb]

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: 0453-reflection-matching-wip.patch is work in progress. See both http://dev.clojure.org/display/design/Reflection+API and http://dev.clojure.org/display/design/Matching for design notes

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: [file:bcLZTE19mr36vzeJe5cbCb]

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: The third patch (dated Oct 14) is per conversation with Rich, plus one enhancement:

  • main namespace is clojure.reflect, and aims to be platform-generic.
  • java specific bits are in the same namespace but a different file reflect/java.clj

I believe this organization helps keep things separate for implementers without punishing users, who run on a single platform at a time and want a single namespace.

Comment by Assembla Importer [ 15/Oct/10 12:30 AM ]

stu said: Updating tickets (#263, #305, #315, #364, #453)





[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-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-444] Infinite recursion in Keyword.intern leads to stack overflow Created: 28/Sep/10  Updated: 28/Sep/10  Resolved: 28/Sep/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


 Description   

Keyword.intern keeps a table of SoftReferences to keywords, always preferring the existing one to making a new one. However, if the SoftReference has been pulled, the code tries to recurse as its way of "trying again."

Unfortunately, the way the logic is written, the code will work exactly the same way the second time around (and the third... it's turtles all the way down the stack) and eventually have a stack overflow.

I found this in a long running web service that was using XML RPC and then "keywordizing" the keys of the result maps. Within an hour or so, it would blow up on this. From my read of the code, there is no work around once you're in this state since the expired reference is stuck in the keyword table.

Patch coming momentarily.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 9:30 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/444
Attachments:
keyword-intern.diff - https://www.assembla.com/spaces/clojure/documents/a1Rf2-YY0r37R9eJe5cbLA/download/a1Rf2-YY0r37R9eJe5cbLA

Comment by Assembla Importer [ 28/Sep/10 9:30 PM ]

tomfaulhaber said: [file:a1Rf2-YY0r37R9eJe5cbLA]: bug fix

Comment by Assembla Importer [ 28/Sep/10 9:30 PM ]

richhickey said: I don't think this analysis or patch is quite right.

The Util.clearCache call at the top of intern should delete the dead entry. I guess there can be a window in which the softreference hasn't yet been enqueued, if it has any extent you could spin and see your stack overflow.

But slamming in a new entry without the putIfAbsent logic risks an alias (two keywords with the same name) in a race condition.

Better to explicitly remove the entry (not waiting on the softref queue) and recur.

I'll fix.

Comment by Assembla Importer [ 28/Sep/10 9:30 PM ]

richhickey said: (In [[r:167a73857a746e8dbeeb6d9ea8f99083aca7dc69]]) don't rely on softref queue, explicitly remove dead entry when found, fixes #444

Branch: master





[CLJ-443] Add zero-arity body to comp function Created: 28/Sep/10  Updated: 11/Oct/10  Resolved: 11/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   

The comp function should return the identity function should it be called with zero args. Currently there is no zero-arity form. I talked a little more about this at http://blog.fogus.me/2010/08/18/monkeying-with-clojures-comp-function/. Patch attached.



 Comments   
Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/443
Attachments:
comp0.diff - https://www.assembla.com/spaces/clojure/documents/aHXT_cYXqr35r8eJe5cbLr/download/aHXT_cYXqr35r8eJe5cbLr

Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)

Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)





[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-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-439] Automatic type translation from Integer to Long Created: 27/Sep/10  Updated: 28/Sep/10  Resolved: 28/Sep/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


 Description   

In some cases types can get lost:

(def i1 (Integer. "100"))
(def i2 (Integer/valueOf 100))

[(class i1) (class i2)]
==> [java.lang.Integer java.lang.Long]

;; expected ==> [java.lang.Integer java.lang.Integer]

;; But: (class (Integer/valueOf 100)) ==> java.lang.Integer

Works correspondingly for Floats, which become Doubles.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 1:39 PM ]

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

Comment by Assembla Importer [ 28/Sep/10 1:39 PM ]

richhickey said: (In [[r:cc8372f12074b4cccbdd9cde3cfacfae069c57d3]]) don't coerce pre-boxed Integers and Floats to Longs/Doubles, fixes #439

Branch: master





[CLJ-437] Bugs in clojure.set/subset? and superset? for sets with false/nil elements Created: 21/Sep/10  Updated: 12/Oct/10  Resolved: 12/Oct/10

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

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

Approval: Ok

 Description   

clojure.set/subset? and /superset? fail on sets with false or nil elements, since they use the input sets directly as a membership predicates:

user> (clojure.set/subset? #{nil} #{nil 2 3})
false

http://groups.google.com/group/clojure-dev/browse_frm/thread/c9c2bebd64803959/e61bbc0d148ecd7b#e61bbc0d148ecd7b



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

Converted from http://www.assembla.com/spaces/clojure/tickets/437
Attachments:
fix-subset.diff - https://www.assembla.com/spaces/clojure/documents/arGxjOXBqr3792eJe5cbLA/download/arGxjOXBqr3792eJe5cbLA

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

stu said: Updating tickets (#429, #437, #397, #420)

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

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





[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-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-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-430] clojure.java.io URL Coercion throws java.lang.ClassCastException Created: 02/Sep/10  Updated: 11/Oct/10  Resolved: 11/Oct/10

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

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

Approval: Ok

 Description   

I noticed a bug in the clojure.java.io namespace at Line 57 in extend-protocol Coercions URL

URL
   (as-url [u] u)
   (as-file [u]
     (if (= "file" (.getProtocol u))
       (as-file (.getPath u))
--      (throw (IllegalArgumentException. "Not a file: " u))))
++      (throw (IllegalArgumentException. (str "Not a file: " u)))))
</code></pre>
Right now it throws the error:
<pre><code>
Caused by: java.lang.ClassCastException: java.net.URL cannot be cast to java.lang.Throwable
        at clojure.java.io $fn__7354.invoke(io.clj:56)
        at clojure.java.io $fn__7328$G__7323__7333.invoke(io.clj:34)

Clojure version: 1.2



 Comments   
Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/430
Attachments:
0430-java-io-url-coercion.patch - https://www.assembla.com/spaces/clojure/documents/cK6UTSZw4r345CeJe5cbLA/download/cK6UTSZw4r345CeJe5cbLA

Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

stu said: [file:cK6UTSZw4r345CeJe5cbLA]

Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)

Comment by Assembla Importer [ 11/Oct/10 2:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)





[CLJ-429] revamp ant build Created: 26/Aug/10  Updated: 01/Oct/10  Resolved: 01/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   

reduce number of manual steps, improve clarity, add options



 Comments   
Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/429
Attachments:
0429-revamp-ant-build.patch - https://www.assembla.com/spaces/clojure/documents/dlWZ16SsSr36B6eJe5cbLr/download/dlWZ16SsSr36B6eJe5cbLr
0429-revamp-ant-build-for-master.patch - https://www.assembla.com/spaces/clojure/documents/bmkYJaXO4r36NjeJe5cbCb/download/bmkYJaXO4r36NjeJe5cbCb

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

stu said: [file:dlWZ16SsSr36B6eJe5cbLr]

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

oranenj said: I gave this a quick run on top of 1.2.0 and it seems to work. The patch doesn't apply to master though. Is it supposed to?

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

richhickey said: Yes, what about master? I thought we were going to patch master and move changes to branch?

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

stu said: [file:bmkYJaXO4r36NjeJe5cbCb]

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

stu said: first patch is for 1.2.x, second is for master. They both end in the same place, but differ because the 1.2.x branch had some separate (irrelevant going forward) changes in build.xml

Comment by Assembla Importer [ 01/Oct/10 8:53 AM ]

stu said: Updating tickets (#429, #437, #397, #420)





[CLJ-427] Github URL on clojure.org submission guidelines is out of date Created: 18/Aug/10  Updated: 28/Sep/10  Resolved: 28/Sep/10

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

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

Approval: Ok

 Description   

Need to update the git clone URLs on this page: http://clojure.org/patches as it still points to git://github.com/richhickey/clojure-contrib.git.



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

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

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

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

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

stu said: Updating tickets (#421, #427)





[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-425] stop early-loading ancillary libraries Created: 13/Aug/10  Updated: 22/Sep/10  Resolved: 22/Sep/10

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

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

Approval: Ok

 Description   

In RT.java, doInit loads a bunch of libraries on startup:

load("clojure/zip", false);
 load("clojure/xml", false);
 load("clojure/set", false);

This is vestigial from before use/require, and Clojure would load faster if it went away. Would be a small breaking change if anyone uses these libraries with full namespace qualification, and without a prior load/use.



 Comments   
Comment by Assembla Importer [ 22/Sep/10 2:25 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/425
Attachments:
0425-stop-early-loading.patch - https://www.assembla.com/spaces/clojure/documents/bwUWm2Rg0r349IeJe5cbLA/download/bwUWm2Rg0r349IeJe5cbLA

Comment by Assembla Importer [ 22/Sep/10 2:25 PM ]

stu said: [file:bwUWm2Rg0r349IeJe5cbLA]





[CLJ-421] clojure tests can fail because of a race in the pprint tests Created: 09/Aug/10  Updated: 28/Sep/10  Resolved: 28/Sep/10

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

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

Approval: Ok

 Description   

When I was doing the pprint tests for printing agents, I typod the code that guarded against races.

Bug was observed by Rasmus Svensson in http://groups.google.com/group/clojure-dev/browse_thread/thread/26b14bcada913247#

Example:

[java] FAIL in (pprint-datastructures-tests) (test_pretty.clj:226)
[java] expected: ((var
clojure.test-clojure.pprint.test-helper/back-match) (tst-pprint 20
(failed-agent)) #"#<Agent@[0-9a-f]+ FAILED: \n \"foo\">")
[java] actual: nil

Patch coming momentarily.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/421
Attachments:
bug421-patch.diff - https://www.assembla.com/spaces/clojure/documents/dzMEaqO-Kr36zXeJe5cbLA/download/dzMEaqO-Kr36zXeJe5cbLA

Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

tomfaulhaber said: [file:dzMEaqO-Kr36zXeJe5cbLA]: The patch to fix the bug

Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

tomfaulhaber said: I've added a patch that should fix the bug. I'll ask Rasmus to confirm.

Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

tomfaulhaber said: Rasmus has confirmed that this indeed fixes the problem. I'd push for this change to go into 1.2 if anything more ends up going in, so that folks can run the tests reliably.

Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

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

Comment by Assembla Importer [ 28/Sep/10 1:00 PM ]

stu said: Updating tickets (#421, #427)





[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-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-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-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-378] Set thread names on agent thread pools Created: 09/Jun/10  Updated: 12/Oct/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: 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)





[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-368] Redefining a function that uses a redifined macro, picks up old macro defintion Created: 31/May/10  Updated: 11/Oct/10  Resolved: 11/Oct/10

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

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

Approval: Ok

 Description   

Redefining a function to use an updated macro definition fails to work correctly, with the function still using the previous macro definiton. This seems to be a regression.

Test case: http://gist.github.com/420007
Bisection : http://gist.github.com/420075



 Comments   
Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/368
Attachments:
0368-dynamic-redef-regression-test.patch - https://www.assembla.com/spaces/clojure/documents/aLXklKBE8r36weeJe5cbLA/download/aLXklKBE8r36weeJe5cbLA

Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

cgrand said: This bug is caused by top-level fns not being numbered anymore. Since their name doesn't change, the classCache in DynamicClassLoader finds the previous defintion, compares the expressions bodies (before macroexpansion) and thus concludes the cache entry is fresh and returns it.

I see two potential fixes:

  • use a deeply macroexpanded body as the value of the cache entry, but this would require building this body – however it isn't enough, see http://gist.github.com/420344: hinting doesn't change value since it's metadata so the unhinted version keeps getting pulled from the cache. This is not an acceptable fix.
  • use (wrapped) byte arrays as values (eg java.nio.ByteBuffer/wrap creates wrappers with value-based equality) but I don't know under which conditions a reload would produce two identical byte arrays – experimentally, types and records Classes are pulled from the cache when reloaded but fns aren't.
Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

stu said: [file:aLXklKBE8r36weeJe5cbLA]

Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

stu said: This issue was already fixed by http://github.com/richhickey/clojure/commit/f47b3d6f028e0370c495383731a449092d0ae451, but I wanted to add a regression test.

Also: the file this regression test lives in is not in the test suite list yet – I already have a commit fixing that queued up as part of #359.

Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

stu said: Patch is just a regression test for fix already made.

Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)

Comment by Assembla Importer [ 11/Oct/10 3:25 PM ]

stu said: Updating tickets (#139, #278, #285, #368, #443, #430)





[CLJ-364] Make PersistentQueue Counted Created: 27/May/10  Updated: 15/Oct/10  Resolved: 15/Oct/10

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

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

Approval: Ok

 Description   

Currently, calling count() on PersistentQueue can take a long time for large queues. PersistentQueue should implement the Counted interface and provide constant time counting by maintaining a count of the number of elements internally.



 Comments   
Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/364
Attachments:
persistent-queue-counted.diff - https://www.assembla.com/spaces/clojure/documents/dNDFA4ADar352MeJe5cbCb/download/dNDFA4ADar352MeJe5cbCb
0364-count-pq-with-tests.patch - https://www.assembla.com/spaces/clojure/documents/aGuueU15Gr347DeJe5cbLA/download/aGuueU15Gr347DeJe5cbLA

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

importer said: [file:dNDFA4ADar352MeJe5cbCb]

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

digash said: * This also affects agents performance.

    • Agents use count method in en-queuing methods.
    • It causes large slow down on sent to agent when queue size is large.
Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

stu said: [file:aGuueU15Gr347DeJe5cbLA]

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

stu said: Second patch subsumes first, adds tests

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

stu said: Updating tickets (#263, #305, #315, #364, #453)





[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-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-315] Add support for running -main namespace from clojure.main without AOT Created: 24/Apr/10  Updated: 15/Oct/10  Resolved: 15/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   

This patch allows clojure.main to accept an argument pointing to a namespace to look for a -main function in. This allows users to write -main functions that will work the same whether the code is AOT-compiled for use in an executable jar or just run from source.

Discussed on the mailing list: http://groups.google.com/group/clojure/browse_thread/thread/bcf3c7c0e002601e



 Comments   
Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/315
Attachments:
0001-Add-support-for-running-main-namespaces-from-clojure.patch - https://www.assembla.com/spaces/clojure/documents/a4lY7et7Sr34T_eJe5dVir/download/a4lY7et7Sr34T_eJe5dVir
0002-Add-support-for-running-main-namespaces-from-clojure.patch - https://www.assembla.com/spaces/clojure/documents/azCC8mvcmr35JueJe5cbLA/download/azCC8mvcmr35JueJe5cbLA

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

technomancy said: [file:a4lY7et7Sr34T_eJe5dVir]: Patch to provide above functionality

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

scgilardi said: I like the idea and the code and I'd be happy to see this added to clojure.main. I'll test this evening and comment again.

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

scgilardi said: [file:azCC8mvcmr35JueJe5cbLA]: updated patch with minor doc changes

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

scgilardi said: I tested Phil's patch and found it to work well. I made some small changes to the documentation strings and the order of function definitions in the source file. I've attached my revised patch (which includes Phil's as a separate commit).

I think Clojure users will find this a valuable addition and I think it would be a good idea to include it in 1.2 if possible.

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

technomancy said: Steve's changes make sense. It would be great if this could be merged for the 1.2 RC.

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

technomancy said: Any chance we could move this forward? Considering Steve is the original author of clojure.main, I think it makes sense to apply his patch.

Comment by Assembla Importer [ 15/Oct/10 4:30 AM ]

stu said: Updating tickets (#263, #305, #315, #364, #453)





[CLJ-305] *out* has a misleading docstring Created: 21/Apr/10  Updated: 15/Oct/10  Resolved: 15/Oct/10

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

Type: Defect
Reporter: Anonymous Assignee: Daniel Solano Gómez
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

The docstring for out reads:

Defaults to System/out

Like in, it should read something like:

Defaults to System/out, wrapped in an OutputStreamWriter



 Comments   
Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/305
Attachments:
out_docstring.patch - https://www.assembla.com/spaces/clojure/documents/aT8Ql0tm8r35jXeJe5cbCb/download/aT8Ql0tm8r35jXeJe5cbCb
0305-out-docstring-resolved.patch - https://www.assembla.com/spaces/clojure/documents/cS--Gg16Or36XAeJe5cbLA/download/cS--Gg16Or36XAeJe5cbLA

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

stu said: [file:cS--Gg16Or36XAeJe5cbLA]

Comment by Assembla Importer [ 15/Oct/10 1:30 PM ]

stu said: Updating tickets (#263, #305, #315, #364, #453)





[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-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-286] *out* being used as java.io.PrintWriter Created: 29/Mar/10  Updated: 14/Oct/10  Resolved: 14/Oct/10

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

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

Approval: Ok

 Description   

There seem to be several places in clojure/contrib where out is being used as a java.io.PrintWriter, despite being documented as of type java.io.Writer
Examples are:

  • The reflection warnings from the compiler
  • Exception printouts from contrib.sql

This causes very confusing errors, when out isn't actually a PrintWriter, for example in a swank repl where it is a StringWriter.

I've created a patch for the reflection warnings, but can't submit it here because of political issues.



 Comments   
Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/286
Attachments:
0001-fix-clojure.main-to-not-assume-that-err-is-a-PrintWr.patch - https://www.assembla.com/spaces/clojure/documents/cEQbXEuiar367beJe5d-aX/download/cEQbXEuiar367beJe5d-aX
0286-out-writer-and-better-pst.patch - https://www.assembla.com/spaces/clojure/documents/b4nAPA16Gr34ZzeJe5cbLA/download/b4nAPA16Gr34ZzeJe5cbLA

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

rnewman said: See

https://www.assembla.com/spaces/clojure-contrib/tickets/55-clojure-contrib-sql-expects-*err*-to-be-a-printwriter

and the associated discussion on the group.

Clojure's out is actually intended to be a PrintWriter; it's a documentation issue.

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

djpowell said: [file:cEQbXEuiar367beJe5d-aX]: Fix for related printwriter issue

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

djpowell said: There was also some discussion that out should not be assumed to be a PrintWriter:

http://groups.google.co.uk/group/clojure-dev/browse_thread/thread/99aa2b3263a0b374?hl=en

I've attached a patch for the related issue of err being assumed to be a PrintWriter. I think this is the only place in clojure itself, although there are other places in contrib.

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

stu said: [file:b4nAPA16Gr34ZzeJe5cbLA]

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

stu said: Second patch subsumes first, plus two additions

  • applies the same fix to the new pst function
  • improves usability of pst. pst's defaulting for the single-arity case was the opposite of what I expected, and the resulting error message is spectacularly confusing. Since the defaulting could trivially be extended to meet my expectations (pass an exception) without breaking the existing (pass a depth), I made it support both.

I don't like binding out to err, but the entire print plumbing drives you in this direction.

Comment by Assembla Importer [ 14/Oct/10 2:27 PM ]

stu said: Updating tickets (#182, #286)





[CLJ-285] Make it easier to discover the source of compiler exceptions when two files have the same name in different directories Created: 27/Mar/10  Updated: 11/Oct/10  Resolved: 11/Oct/10

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

Type: Enhancement
Reporter: Anonymous Assignee: Assembla Importer
Resolution: Completed Votes: 0