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.

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.

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

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.

This patch breaks tests.

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

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

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

this patch no longer applies cleanly

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>

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

The patch may apply to 1.1 and 1.2 too.

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

Bump

just juxt, for now

Added patch to back out the deftype print-dup.

Erm, ditto print-method.

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

Ditto print-method.

Replaces the previous patch.

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.

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.

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

Second patch is more idiomatic.

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.

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

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

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.

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.

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

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.

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.

Code & Test patches pushed.

Added patch with fixes and tests.

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

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?

Patch emits BigIntegers using the N notation.

Apply patch after CLJ-799.

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

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.

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

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?

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

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.

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.

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.

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.

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

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

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?

Patch fixes regression and adds some tests.

Clojure alpha7

patch attached

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.

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

With the alpha7 stuff included

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

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"

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.

If someone is supplying a message, will they want the original message like that?

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

commit message is odd, but commit looks right

Clojure 1.2

Fix for partition-by by realising the drop.

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.

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.

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.

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

Updated patch according git workflow.

Patch applied.

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.

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

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.

What's behind clojure.org? Is it a real server we can log into or some kind of hosted app?

Will take this up with GitHub if we continue to have problems.

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.

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.

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.

This worked properly with the beta release

Remove the warning message as well?

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.

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

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

(cl-format nil "~:(~a~)" "")

Patch that fixes the issue and adds tests

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

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.

Patch submitted.

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?

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

This patch does not change the print-method – it turns out that would have been a mistake.

My patch is same as Chouser's except tweaked to make the git gods happy.

Fix in clj-739-a.patch

Please don't zip your patches, thanks!

Got it.

Mac OS X, Java 6

The source of the issue seems to be incorrect treatment of boxed NaN:

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs.

In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:

user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true

So the bug is not that boxed NaN is treated incorrectly, but rather:

user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false

In the original example, since there are more than two args, the primitive looking args were boxed:

user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true

Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc).

If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying.

Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful.

Then there's that -0.0 nonsense...

On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:

user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true

Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.

Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.

Rich, what should the behavior be?

My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link:

http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1

It should work the same as primitive double in all cases

Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.

However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

An updated patch that includes tests for the new functionality.

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)."

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}>

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.

Any thoughts on the open questions part Rich?

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.

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

here is the diff against the current clojure master branch.

re-added patch in proper format

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

Rich: does this need to be rewritten so that BindingInit is a value, or is the mutation of startScope ok?

Patch works against master.

Yes, it is weird to mutate bindinginit with emit-related info. Better to keep a map of bindinginit->label inside doEmit, please.

fixed to use HashMap of labels

Feb 26 patch is good

Added a patch containing the variadic unwound versions of every-pred and any-pred and tests for each.

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.

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).

every-pred should return a boolean by filtering through boolean

any-pred should be some-fn

Updated patch.

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.

Third patch renames any-pred to some-fn to match Rich's naming suggestion.

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!

Let's try this again. All wires appear uncrossed.

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

This has the docstring changes suggested by Rich (i.e. removing "truthy" "falsey" etc.).

IBM JDK on Hudson, http://build.clojure.org/job/ibm-jdk-clojure-build/

Patch 0728-reify-exceptions-1.patch.gz changes the test to not check for specific types or messages on the exceptions.

This is the only thing holding us back from a successful build on IBM JDK.

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.

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.

Rich, do you like option #1 in the description, or option #2 + #3, or none of the above?

Any thoughts on this?

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.

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?

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.

Patching both this and RestFn in a single go seems best.

We should have a discussion about what might constitute a 1.2.1 on the dev list.

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.

The second commit has the windows-line-ending problem. Anybody know how to fix this in an automated way?

The same patch as Paul's, with unix line-endings

Stuart: See attached patch run through the dos2unix tool

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

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?

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.

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()

Thanks Rich. I will take this one and get a patch in hopefully today.

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?

looks ok at first glance

New patch 0702-fix-npe-in-nil-case-2.patch.gz adds a test.

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.

Oops. Sorry. Will fix.

waiting on Stuart's patch tweak

Patch 0702-fix-npe-in-nil-case-3.patch.gz replaces previous patches, but preserves correct commit messages and authors.

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

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.

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

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

Affects Clojure from 1.2.0 to 1.3.0-alpha4

Stu, can you please verify?

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)))

Thanks for the fix! Works great.

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]

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.

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.

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.

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

Audit completed; results and recommendations communicated privately.

Verified Aaron's fixes.

Moved to a google doc

https://docs.google.com/document/d/1PiJ4O2ZGWfReKNcG5ON8zrRZVYsFQiKrQRoduAK6upQ/edit?hl=en&authkey=CL7U4ugD

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.

Michael, can you review and edit with any feedback you have about the process? Thanks!

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

Thanks, I've updated with your document.

Doc is here: http://dev.clojure.org/display/design/JIRA+workflow

Please Screen

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

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.

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.

This one-line patch resolves the issue.

OK, this is ready to be applied to master.

Replaces previous patch.

• fixed SCM URLs in pom.xml

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?

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

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.

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

This fixes a few more testing issues.

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.

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

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

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

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