CLJ-852-test.patch
reproduces this issue on master

CLJ-852-test.patch initially failed to use (is ...) leading to ugly output on failure.

CLJ-852-rfc-fix.patch

add special cases for "int"…"boolean" to tagToClass()

This causes the test to pass, however, there are still some open questions:

Why does tagToClass first call maybeClass and then possibly overwrite the result it gets back without even looking at it? This strikes me as surprising. I would have expected a check for null, and then only running the if/else chain of special cases if maybeClass failed to return a non-null result.

The suppressed exception in maybeClass commented with "//aargh" makes me wonder if maybeClass and tagToClass are factored correctly.

This looks reasonable to me, but given the number of places tagToClass gets called I would definitely want Rich to take a look.

See also
http://groups.google.com/group/clojure/browse_thread/thread/f1672a7e09c34721

CLJ-852-fix.patch removes the RFC comments

This fixes the issue. It appears to me like the cases "int" .. "boolean" (i.e. the non-array primitive types) were simply forgotten in tagToClass().

It's not impossible that tagToClass is being misused somehow ant that's the cause of the problem, but I see no evidence for that. (I'd need a better understanding of the code base to make that determination – mind reading skills would help too.)

It's also conceivable that maybeClass() should be doing the resolution from primitive type to Class object, but that doesn't seem likely to me.

clj-852-patch2.txt is exactly the same as Ben's CLJ-852-fix.patch (including keeping his name and dates on the commits), except:

(1) I moved his new unit tests to an already-existing file metadata.clj, which seemed to have related tests with def and metadata, whereas Ben's patch adds a brand new test file. Not sure whether keeping the number of files down is important or not.

(2) I removed some non-ASCII characters from his commit notes.

Both his patch and this newer one apply cleanly to latest master as of Feb 22, 2012. Neither adds any new errors or warnings to compilation or tests (unless you apply the new test part of the patch without his proposed compiler fix, where the new test does fail as expected). No docstrings need changing that I can think of. Ben and I have both signed CAs. Patch status is "Code and Test".

Screened. The addition seems harmless, merely recognizing more tags in tagToClass.

David, the patch for CLJ-873 seems to be more general than this, i.e. it allows the symbol / to be used in any namespace, not only in clojure.core and cljs.core.

I checked with David Nolen, and he agrees that the patch for CLJ-873 fixes not only this issue, but also enables the symbol / to be used in any namespace.

Added updated patch based on fixes to CLJ-871. This patch should be applied AFTER the patch for the latest patch for CLJ-871.

I've just checked the code: send uses binding (which calls push-thread-bindings, creating a new Frame with non-null prev) inside which the binding-conveyor-fn captures the current frame. When the binding-conveyor-fn runs, the old Frame is restored, and since the wrapped function sends again, a new thread-binding is pushed on top of that.

Hm, one way to get rid of the issue was not to capture the "real" current Frame in the binding-conveyor-fn but only a shallow copy, i.e., a Frame with the same bindings but no prev. I've tried that out, all tests still pass, and the example above won't grow heap without bounds. Patch attached.

Screened.

My fix had some small issue as David Miller pointed out correctly in

https://github.com/clojure/clojure/commit/12f07da889819bc5613546ec223e97ac27c86dbf#commitcomment-867608

Attached is a patch that fixes the quirk.

Here's the fix for the fix.

all

Make patch file per http://clojure.org/patches. Also sent in my CA.

Any reason why this hasn't been applied yet? It's a pretty serious error. Just wondering if I've submitted anything incorrectly. Thanks.

CLJ-886-fix2.patch includes Jeff Palmucci's fix, plus another one found while enhancing clojure.java.io's unit tests to fail with the original code. Tested on Mac OS X 10.6.8 with java 1.6.0_29 and Ubuntu Linux 11.04 with JVM 1.7.0_02.

Andy's fix is good. Please use the fix2 patch instead of the original.

Screened. Patch applies successfully and the tests seem to be complete.

Attached 872-add-prop-lookup.patch. It's a first cut that Fogus and I worked on. It needs tests.

Tests for prop lookup.

Ready for screening.

Can you explain the compiler change? It appears to implement the desired functionality, but also to disable a code path that allows keywords for field lookup (the block at line 892 can no longer be reached by a keyword).

Clojure has for a while secretly allowed kw lookup for field access. However, in the discussions with Rich and Clojure/dev at http://dev.clojure.org/display/design/Unified+ClojureScript+and+Clojure+field+access+syntax it was decided to use -prop across CLJ and CLJS thus obviating the need for kw access.

I have tested patch locally with no Joda, 1.6.2 Joda, and 2.0 Joda. All work as intended in simple invocations.

Seeking feedback on

• UTC at the door. Goal is to stay out of localization business. Is that a good goal, and is that the right way to achieve it?
• idiom for dynamically loading code based on whether Joda on classpath
• idiom for conveying maven classpath into the ant part of the build. People using ant will have to configure path locally to include Joda.

using a bindable var for a read time setting is kind of a drag stuff like:

(binding [*instant-reader* some-other-instant-reader]
#@2011-11-04T14:42Z)

will not have the desired effect, so then what are the use cases for the bindable var? should the repl bind it? should the compiler?

dates should always be read and printed as UTC

It seems like it would be nice to have time-delta literals as well. My usage cases deal more often with time-deltas applied as intervals or offsets in time. I presume the current scheme doesn't allow for that but could be adjusted. Is that outside of the scope of this discussion?

> UTC at the door.

No. Offsets are not localization, just arithmetic.

>idiom for dynamically loading code based on whether Joda on classpath

We'll need more explicit mechanisms for determining the programmatic representation. Dynamic Joda is just to avoid Joda as hard implementation dep.

> People using ant will have to configure path locally to include Joda.

People using ant = me.

This patch implements instant literals of the form #@yyyy-mm-ddThh:mm:ss.fff+hh:mm using only classes available in the JRE.

clojure.instant provides instant-readers producing instances of three different JDK classes. These functions accept a string representing a timestamp See (doc clojure.instant/parse-timestamp) for details.

• read-instant-date (java.util.Date)
• read-instant-calendar (java.util.Calendar)
• read-instant-timestamp (java.sql.Timestamp)

By default *instant-reader* is bound to read-instant-date.

print-method and print-dup are provided for all three types.

Rough bits include:

I'm not yet certain about the exact public interface of clojure.instant. It's clear that read-instant-* need to be visible. It also seems likely that parse-timestamp and validated could usefully support alternate implementations for *instant-reader*.

fixup-offset and fixup-nanos are ugly warts necessitated by Java's pathetic built-in support for dates and times (possibly exacerbated by my own misunderstandings of the same).

Unit tests are very basic. For example, I'm not testing validated except in the good case where everything is valid.

See also https://github.com/bpsm/clojure/commit/753f991151847df53d624f7c09b7113cd2321793

I've made a few trivial fixes to doc strings, visible on my github branch. Those changes will be included when I re-roll the patch it to incorporate any feedback.

CLJ-871-data-readers-1.patch

An alternative approach. The dynamic Var *data-readers* maps keywords to reader functions, triggered by reader syntax #:keyword. Default #:instant reader takes a vector like [year month day hrs min sec ms], all components optional.

Pros: No string parsing. Extensible.

Cons: Clojure 1.2 used the #: syntax to print records.

Oddly, I made another case for this. My bad. Attached is the updated patch based off of Ben's work. This patch is merged into the tagged literal feature as implemented in v1.4-alpha4. I also fixed a fence-post error and very minor doc problems. The parser currently defaults to constructing j.u.Date instances of the following form:

date-year   = 4DIGIT
date-month      = 2DIGIT  ; 01-12
date-mday       = 2DIGIT  ; 01-28, 01-29, 01-30, 01-31 based on
                         ; month/year
time-hour       = 2DIGIT  ; 00-23
time-minute     = 2DIGIT  ; 00-59
time-second     = 2DIGIT  ; 00-58, 00-59, 00-60 based on leap second
                         ; rules
time-secfrac    = '.' 1*DIGIT
time-numoffset  = ('+' / '-') time-hour ':' time-minute
time-offset     = 'Z' / time-numoffset

time-part            = time-hour [ ':' time-minute [ ':' time-second [time-secfrac] [time-offset] ] ]

timestamp       = date-year [ '-' date-month [ '-' date-mday [ 'T' time-part ] ] ]

Or in other words, RFC 3339 .

2 problems:

1. You can't specify an alternate inst reader in data_readers.clj. Doing so causes an opaque error when Clojure starts. This is a flaw in CLJ-890

2. *data-readers* is intended to be a map of symbols to Vars, not symbols to functions. This doesn't really matter.

New patch CLJ-871-with-defaults.patch.

Fixes problems described in previous comment by adding default-data-readers which can be overridden by *data-readers*. Also adds documentation for *data-readers*.

divisible? and indivisible? should be normal functions, not macros. They're used only in leap-year? – it would be pretty simple to use zero? and mod? directly there.

There may be a couple different ways to fix this. I think what would work is adding a Util.hashEquiv method that has the same equality semantics as Util.equiv and using that to hash keys instead.

I'd be glad to create a patch for that.

This ticket can be closed now. It is fixed by https://github.com/clojure/clojure/commit/b5f5ba2e15dc2f20e14e05141f7de7c6a3d91179

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

+1

The ability to debug is greatly degraded when locals on lower stack frames have already been cleared. I'll apply this patch and will test in the next few days.

It seems we might have a number of new debug settings ahead. Are there other approaches besides filling the core namespace? Names in a different namespace? A single name in core that points to a map?

We could have this triggered by system properties. We've talked about having a clojure.debug system property; it would make sense to have this set a number of other properties including one specifically for locals clearing.

From a user perspective, I would like to be able to control locals clearing on a fine grained basis. In slime, for instance, C-u C-c C-c could be used to compile with locals disabled. This would improve the possibilities for debugging code that required locals clearing to work properly.

To me this would suggest the use of a var (though the var could be held in a map).

clojure.settings/disable-locals-clearing sounds good to me

I much prefer either the clojure.settings namespace (or similar), or the map of options. I think what Hugo is describing as an important use case to keep in mind, but also, if Clojure itself has the facilities to handle this functionality, I don't see any reason to tie it to Java's property system. If compilation options proliferate, it doesn't seem unlikely to me that multiple target Clojures (CLR, CLJS) might also use similar flags, and by keeping it in Clojure it could make that simpler to deal with.

Hugo also pointed out to me privately that we could also make these vars be initialized from system properties, which seems to me like it would get a lot of the convenience you would get from having this based on system properties.

Finally, I'd just like to say that I think clojure.settings/locals-clearing, default: true sounds a little better to me. It would make code using that variable a little clearer because then the enable/disable matches the true/false value it holds.

The proliferation of dynamic Vars in core has concerned me too. Java System Properties are nice for Java interop, but they can only have String values and they lack thread-local binding semantics.

A dynamic map would work, but binding it correctly is more complicated:

(binding [*settings* (merge *settings* {...})] ...)

This could be mitigated with a macro.

A namespace for settings is convenient for documentation purposes, but what about the cost of dynamic Vars? If they're only used at compile-time, it doesn't really matter, but a single Var lookup is still cheaper than many.

Are we really concerned about thread-locality when dealing with compiler settings? Concurrent compilation already has issues with transactionality, so I got the impression that it's not encouraged. I don't see the string limitation being a problem here, but perhaps there are other use cases where it would be annoyingly limiting?

Phil wrote: Are we really concerned about thread-locality when dealing with compiler settings?

We aren't necessarily concerned with thread-locality, although the compiler itself a lot of dynamically-scoped Vars internally. For external use, all we really need is scoped, temporary settings, so Java system properties would work with some helper macrology like with-properties.

The string limitation isn't a problem right now, but it could be annoying if we added something like CL feature-test expressions.

Inverts the logic of the previous patch and uses enable-locals-clearing.

I'm unclear as to what the consensus is on how to avoid var proliferation.

This patch implements compiler-options as a map with the :locals-clearing key.
The initialisation is via the value of the clojure.compile.locals-clearing system property.

0001-add-compiler-options-map.diff

Implements compiler-options as a map, with a :locals-clearing key, initialised from the clojure.compile.locals-clearing system property.

Just a note that all 3 patches attached right now fail to apply cleanly using 'git --keep-cr -s < patchfile.txt' to latest master as of March 9, 2012. Any consensus on which of these approaches to take? I can update that one if so.

https://github.com/clojure/clojure/commit/4036c7720949cb21ccf53c5c7c54ed1daaff2fda

Per RH's request, I tested this with CDT, and confirmed it works there.

(marked up code snippet in description to display as code.)

(This incorporates a slightly modified version of Paul Michael Bauer's
checked-exception-regression-test patch.)

ClojureException is an RTE. RTEs that originate from Clojure all are
of this type. This isn't strictly necessary for this proof of concept,
but it seemed prudent since it seems that the lack of specificity resulting
from using plain RTE everywhere got us into this mess in the first place.

WrappedException is a ClojureException. It is used when Clojure is
wrapping the underlying cause to work around Java's obsession with
checked exceptions. It's always the underlying cause of
WrappedException that is relevant to catchers.

Util.runtimeException(s) now returns a ClojureException
This is not strictly necessary for this patch to work

Util.runtimeException(s,e) now returns a WrappedException.

Util.runtimeException(e) now retuns a WrappedException when e is not
already a RuntimeException.

clojure.core/treye is a macro built on top of the compiler-provided
special form try. (A real solution to this problem would involve
altering the try special form itself, but this ist just a trial balloon
to figure out if this is the right way to go.)

Paul Michael Bauer's try_catch.clj uses treye in place of try and passes.

> http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

seems like refactoring Reflector.java to rethrow using this approach
is preferable to some wrapping/unwrapping scheme

(edit: missed Kevin's post)
http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

I'd rather see Reflector.java modified to use this "chucking" technique than have a special try form that specially unwraps an exception and re-throws.

Maybe there's a need for WrappedException, but since it's not immediately necessary if we fix Reflector, then I'd rather see it in a separate commit.

(I'm swamped at work today and don't have time to fix Reflector myself right now)

I added Util.throwUnchecked and Util.throwCause (also Util.declareThrows and Util.invokeThrowing) as justified in https://groups.google.com/d/msg/clojure-dev/4QynV81W0Qk/rIN9fYUK-AkJ
I've used this to fix the invoke* methods in Reflector. Please review.

There are a lot of other instances in the code, where checked exceptions are runtime-ified, but that's a separate issue.

Applying the patch CLJ-855_throw_undeclared_checked.patch produces java that can not be compiled in four places:

catch (Exception e)
  {
  Util.throwCause(e);
  }

This leaves the compiler complaining about a missing return.

catch (Exception e) 
  {
  return Util.throwCause(e);
  }

compiles.

Also, the patch doesn't actually cause try_catch.clj to pass. I suspect the RuntimeException being caught by try_catch.clj is not, in fact, originating from one of the four points you have corrected. (Indeed, I discovered something similar when working on my patch, but it was late, and I ended up solving it in such a way that I didn't have to find the actual source of this particular wrapped exception.) I'm still investigating...

@Herwig thanks.
A few issues with the patch:
1. doesn't build. Use "return Util.throwCause(...)" instead of just "Util.throwCause(...)" Otherwise the compiler complains about a missing return statement.
2. could you observe the whitespace conventions in the rest of the Java source files (tabs for indentation)?
3. The semantics of throwCause are different than before. Previously, we threw the Throwable only if t wasnt 1) null 2) an instance of Exception or 3) instance of Error. Subtle, and maybe it's a non-issue?

The wrapping RuntimeException that causes try_catch.clj to fail is actually coming out of Compiler.eval(Object,boolean) without involving Reflector at all.

CLJ-855 appears to be about more than just oddness in Reflector.java. My guess is it requires a more general solution.

Oh, it seems I was too fast with the second commit. That's kind of embarrassing, sorry about that.
Please only consider the first commit of the patch, where I implemented the chucked exception helpers.

I had considered the issue with only rethrowing causes that are instances of Exception or Error. I dismissed it, because I considered it an artifact of not having chucked exceptions before. The throwCause is for unwrapping InvokationTargetExceptions and such. The only case where it could unwrap differently than the original code, is when having causes other than Exception or Error. i.e. a third subclass of Throwable, which is mostly unheard of (but should probably be unwrapped too).

Rich, do you remember, if there are any further implications to the catch idiom in Reflector?

catch(Exception e)
    {
    if(e.getCause() instanceof Exception)
      throw Util.runtimeException(e.getCause());
    else if(e.getCause() instanceof Error)
      throw (Error) e.getCause();
    throw Util.runtimeException(e);
    }

Regarding the actual source of the wrapping (Compiler.eval): It showed up in my `rgrep catch(Exception`, but I considered it part of the "other issue" (of general refactoring). In fact, I think it won't show up, mostly, because people noticed this issue when having reflective calls in clojure code, which don't use eval. So the test case is probably testing another source of RuntimeException wrapping.

This shows, IMO, that most occurrences of `throw Util.runtimeException` should be replaced by `return Util.throwUnchecked`, but probably not all. This will require some scrutiny and careful testing. We need to consider every `catch (Exception e)` in the clj code base.

I'll take a stab at it, tonight (CEST), also testing properly this time (sorry again, had some troubles with my build system yesterday)

I've added a unit test which provokes this issue in the Reflector, as opposed to in the Compiler by way of eval. You can browse it here:

https://github.com/bpsm/clojure/commits/CLJ-855

I've refined my patch using the "try unwraps" strategy. The refined patch renames Clojure's try special form to try* and provides try with transparent unwrapping as a macro on top of try*. The curious may look here:

https://github.com/bpsm/clojure/commits/CLJ-855-try-unwraps

I'll post new patches after I've had some sleep.

I intend to try my hand at the "sneaky throws" strategy that seems to be all the rage here. My experimentation with Herwig's patch showed that it will require investigating some failing unit tests. I believe it will prove a more invasive change than "try unwraps", but may be worth it. A nice challenge.

CLJ-855-try-unwraps.patch:

1. Teaches the Java portion of Clojure core to use WrappedException (a RuntimeException) when wrapping checked exceptions for re-throwing.
2. Changes try to transparently unwrap (only!) WrappedException, passing the cause on to be handled by one of its catch clauses. This appears to solve CLJ-855.
3. The original behavior of the try special form (no transparent unwrapping) is still available under the name try*.

CLJ-855-sneaky-throw.patch

Using "sneaky throw" allows us to effectively rethrow checked exceptions without needing to declare or catch them. In effect, we're back to where we were before wrapping them all up in RTEs (8fda34e4c77...), but without needing to pollute seemingly every method signature in the Java portion of Clojure core with "throws Exception"

This patch was less work than I expected (thanks be to sed) but I'm not as confident in its correctness as I am in that of the try-unwrap variant. I can't claim to have really groked Clojure's approach to exceptions.

I don't understand why Reflector gives preference to rethrowing the cause of the exception it catches (especially now that we're no longer wrapping all over the place.)

I also don't understand why Var.dissoc returns the exception it catches rather than throwing it. (I've left a big fat TODO comment there, which should go from any final version of this patch, should it be accepted.)

Another option, not to make things even more complicated, is to revert the original commit. Perhaps I'm missing the rationale, but...

Checked exceptions are irrelevant to Clojure code, since they are a fabrication of the Java compiler.

Using Clojure code from the Java side could be confusing if you call invoke on IFn and get a checked exception even though it is not declared with a throws clause. It is also confusing to get checked exceptions wrapped in one or more layers of RTEs. Was it not sufficient the way it was before having IFn declare "throws Exception"?

CLJ-855-sneaky-throw.patch applied. Thanks Ben!

Windows 7; Java 1.7

Only affects the assert-args macro, which is private in clojure.core. Patch works as advertised.

By the way, since this involves randomness, on occasion it doesn't fail. With the input as given it seems to fail around 80% of the time, but if you want to be sure to reproduce you can add another 0 to the input size.

Thanks for the update. I was able to reproduce with the extra zero. Moving this ticket to Release.Next so that it will ship with 1.3.

Sorry for the delay. Here is a fix and a reduced test case.

I have checked behavior with an independent test

https://github.com/clojure/test.generative/commit/b1350235eb219b76f5b7c4cc21c8255f567892b3

which looks good, but don't have context to evaluate the code (particularly the array allocation) in the time I have available today.

The array alloc looks suspicious:

+ Object[] newArray = new Object[2*(array.length+1)]; // make room for next assoc
+ System.arraycopy(array, 0, newArray, 0, 2*count);

should it not be array.length + 2, (or 2*(count + 1), whichever?

Thanks Rich for spotting this copy&paste error.

I attached an updated patch.

The problem reported by Alan was double:

• the misplaced removedLeaf.val = removedLeaf was causing the global count to be incorrect
• the missing array copy in ensureEditable was causing the seq returned by (keys m) to be mutated (shortened) and this is why all values were not dissoced.

Mutable code is hard, one should invent a cool language with sane state management

New tests continue to pass.

It seems to me that the allocation in the second path is too big by two in some cases (e.g. it gets called in the dissoc! path when the array needs to be copied, but not get bigger). But this might be considered innocuous. The same method is called in the assoc! path where the +2 is needed, so avoiding two objects worth of allocation would require a more substantial patch.

The larger array allocation is similar to the one performed in BitmapIndexedNode.ensureEditable. My line of thought behind this heuristic is that the copying dominates the allocation and that I prefer one slightly larger array than having to allocates two arrays in case of growth.

Anyway it's on collision nodes so it's a rare occurence: I won't bother arguing further if switching to a more conservative allocation helps the patch landing in 1.3.

Note that the attached patch (incidentally) depends on the patch for CLJ-789.

Slides from Clojure Features talk given at Conj 2010

I've uploaded my features talk slides. It has a nice outline for you to start with. If you would, start a confluence page from this. The entire job of documenting this will have to include auditing the main clojure.org docs as well.

I have put a draft of this (with a couple rounds of feedback) at http://dev.clojure.org/display/doc/Documentation+for+1.3+Numerics

first pass of this is done. Not to say it couldn't be done more thoroughly...

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

cemerick said: Here's an implementation of that IFn wrapper that rethrows Exceptions as RuntimeExceptions: SafeFn.java

This makes sense to me, but perhaps not everyone. Thoughts?

See http://dev.clojure.org/display/design/Improvements+to+interop+from+Java for a discussion of these changes.

While the title of this ticket is broad, it looks like the actual discussion is about rethrowing RuntimeException. Clojure 1.3 and later solve this problem a different way, by not declaring Exception on the relevant interface points.

Please add tickets for the other items discussed on the wiki as appropriate.

Is this the same issue as CLJ-806? If so, it might be nice to close one with a comment that it is a duplicate of the other. Given CLJ-806 has no patch, and this one does, perhaps preferable to close CLJ-806?

John, you are right. I didn't find it searching for "test are" earlier today; now I do.

I did as you've suggested: closed CLJ-806 in favour of this one.

I've tested this patch by hand, and it seems to work fine, and the code looks correct. I have tried several ways to add a unit test to verify this fix, but haven't found the right way to do it, if there is one.

I'm working on it...

This patch obsoletes the former on (I'm gonna delete the old one). It includes:

• an extended fix that catches also the zero-arg given case

• a fix for a testcase in test/clojure/test_clojure/java_interop.clj which only passed because of the bug this issue is all about

• a currently #_-disabled test case for testing `are'. It is disabled, because it fails when run with "ant test". However, it works just fine when run inside some REPL. I presume that's because ant runs inside its own JVM instance and thus makes catching exceptions from macroexpand/eval forms impossible?!?

Another minor update. Now (are [] true) is allowed (but meaningless). In the previous patch, you got a "divide by zero" in this case.

Patch 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch applies, builds, and tests cleanly as of March 9, 2012 latest master. One author Tassilo has signed CA.

I can't make the test work either. The patch looks good. Just remove the broken test instead of leaving it commented out.

clj-931-error-on-bad-are-syntax-patch2.txt same as previous one, except removes commented out test. Applies, builds, and tests cleanly to latest master.

Screened.

Attached a patch with the minor code fix.

Screened. I can't demonstrate this code being called by the reader, but it seems to be correct.

Sorry to disturb things after this has been screened, but clj-924-unicode-invalid-digit-patch2.txt has some added tests that cause the problem to occur, as well as one other similar one nearby in LispReader.java, which is also fixed, in addition to Cosmin's fix.

clojure-dev posting with more details (and presumably forthcoming discussion): https://groups.google.com/d/topic/clojure-dev/bTNX4pt_b4w/discussion

Updated patch to fix the issue David Powell brought up on the dev list, so that "+" in a URL does not get translated to " " in a File.

Vetted & assigned to "Approved Backlog" in preparation for next release.

All

Rich fixed: https://github.com/clojure/clojure/commit/535907eb2be47eaee0c385ae16436f39d52ffa96

test for fix already made

It seems to me that there are four approaches one might take to address this.

1. Document the current undefined behavior of min and max in the presence of NaN.
2. Define that min and max will return NaN if any argument is NaN.
3. Define that min and max will ignore any NaN arguments.
4. Define that min and max will throw an exception if any argument is NaN.

1 Document current behavior as undefined

This requires no changes in the implementation, but it doesn't strike me as a desirable resolution. Why unnecessarily codify clearly confusing behavior?

2 Make NaN contagious

Define min and max to return NaN if and only if at least one of their arguments is NaN. This seems most in keeping with the (admittedly perverse) behavior of NaN as specified.

See JLS 4.2.4 Floating Point Operations:

An operation that overflows produces a signed infinity, an operation that underflows produces a denormalized value or a signed zero, and an operation that has no mathematically definite result produces NaN. All numeric operations with NaN as an operand produce NaN as a result. As has already been described, NaN is unordered, so a numeric comparison operation involving one or two NaNs returns false and any != comparison involving NaN returns true, including x!=x when x is NaN.

3 Let min and max ignore NaN arguments

This means that (min a NaN b) would be exactly equivalent to (min a b). It would further imply that (min NaN) would be equivalent to (min), which currently throws an exception.

4 Let NaN cause min and max to throw an exception

Currently min and max throw an exception if given arguments that are not Numeric. One might plausibly argue that NaN is not numeric.

CLJ-868-contagious-NaN.patch

(Implementation corresponds to variant 2 from my earlier comment.)

This implements the third variant described above with one difference:

When when all arguments are NaN we don't throw a exception, but return NaN instead.

(min) and (max) still throw an exception, however.

This fell out of the implementation naturally and I couldn't see a good reason to prefer one behavior to another.

The contagious version of the patch seems to me the right approach, assuming fidelity to Java (e.g. Math/max and Math/min) is the standard.

contagious ok

The fix is not on the current master, and not in 1.4.0-alpha1.

There's no perf benefit for other arities of map, so for those just define in terms of (into [] (map ...))

Why when-let in filterv? Not returning a vector then. You don't even use s.

What was the problem with defining mapv and filterv in terms of reducev? This is what I had in mind:

(defn reducev
  [f coll]
  (persistent! (reduce f (transient []) coll)))

(defn mapv
  ([f coll]
     (reducev #(conj! % (f %2)) coll))
  ([f coll & colls]
     (into [] (apply map f coll colls))))

(defn filterv
  [pred coll]
  (reducev #(if (pred %2) (conj! % %2) %) coll))

Alan: My original thought was that map and filter are boths fns from collection to collection. reduce is not, so making a reducev that pretends reduce is like map and filter is a little confusing.

But looking at the code I like it.

Rich: ok on both points, thanks.

The problem is - what are the semantics of reducev? Is this for the public or an implementation detail? mapv and filterv take the same f and pred as map and filter, but reducev doesn't take the same f as reduce, it requires a very special, implementation-dependent f.

why not implement in terms of reduce (inline reducev)?

I see no benefit in separate reducev

someone other than Stu should screen this

Microbenchmark shows performance improvement:

user=> (dotimes [i 5] (time (dotimes [j 10000] (vec (map inc (range 100))))))
"Elapsed time: 119.799 msecs"
"Elapsed time: 80.959 msecs"
"Elapsed time: 75.242 msecs"
"Elapsed time: 75.622 msecs"
"Elapsed time: 73.646 msecs"
nil
user=> (dotimes [i 5] (time (dotimes [j 10000] (mapv inc (range 100)))))
"Elapsed time: 199.096 msecs"
"Elapsed time: 61.737 msecs"
"Elapsed time: 49.911 msecs"
"Elapsed time: 50.421 msecs"
"Elapsed time: 48.437 msecs"
nil

Screened by the other Stuart.

Simple test case to see the issue.

=> (defprotocol P
     (self [p]))
P
=> (extend String
     P
     {::self identity})
nil
=> (self "foo")
#<IllegalArgumentException java.lang.IllegalArgumentException: No implementation of method: :self of protocol: #'user/P found for class: java.lang.String>
=> (defmacro try-extend-type [c]
     `(extend-type String
        P
        (self [p#] p#)))
#'user/try-extend-type
=> (try-extend-type String)
nil
=> (self "foo")
#<IllegalArgumentException java.lang.IllegalArgumentException: No implementation of method: :self of protocol: #'user/P found for class: java.lang.String>
=> (keys (get-in P [:impls String]))
(:user/self)

Most of this is a non-issue. :self and :foo/self are not the same.

This:

"Inside emit-hinted-impl, only grab name portion of symbols before converting to keyword."

is the only needed change.

Screened.

Tests would be nice, though I can see how getting such a test to work without 3rd party deps might be difficult.

My naive attempts to reproduce the problem failed. For example, this works fine for me:
(System/loadLibrary "hprof")

Nevertheless, the patch applies cleanly and appears to be what Rich asked for, so I'm setting this to "screened".

looks good

patch applies cleanly, tests pass

contains? working with java.util.Sets seems like a good idea

0001-CLJ-838-changes.txt-recoded-as-markdown.patch

This recodes changes.txt as proper Markdown. Text is not hard-wrapped because github's Markdown dialect handles hard line breaks differently than standard Markdown. Also, the original wasn't hard-wrapped either.

0002-CLJ-838-rename-changes.txt-changes.md.patch

This patch just renames changes.txt to changes.md. I didn't want to rename and edit changes.txt in a single patch, not knowing how well git would handle that when it came time to apply the changes.

0003-CLJ-838-add-example-for-Add-docstring-support-to-def.patch

Meikel Brandmeyer provided an example of docstring support in def in response to my query, so I've included a patch to integrate that.

I don't believe Markdown provides a good way of handling the contents section at the start of the document. I've improvised it by just formatting the thing as if it were a code sample.

Is the TOC really necessary? It's not ideal from a Don't Repeat Yourself standpoint. Also, manually maintaining the section numbers also strikes me as busy work? Can we do without?

You can view a rendered version of changes.md here:

https://github.com/bpsm/clojure/blob/CLJ-838/changes.md

patches updated for f0b092b66 "more changes.txt tweaks"

clojure-1.3.0-beta3 and git master, JDK 1.6.0_26, OS X 1.7.1

The privileged names __meta and __extmap are implementation details of types and records and should probably not be viewed as real. In other words, since they are not published as part of the type/record docs, then they may not exist in the future. Having said that, the current Clojure implementation treats any name starting with __ as a special field and doesn't count them toward the field count. As a result, when emitting the MyType.create method (another implementation detail), the fields marked as __ throw off the field count and as a result the bytecode is not emitted properly. I know how to change the code to allow names prefixed with __ and fixing the validation error (not called by those two special names listed above), but I hesitate to say that is the solution
without further reflection.

I think the prudent fix is as follows:

• Allow any legal name except those in the set #{__meta __extmap}
• Fix the #create emission
• Documentation should be changed to reflect that the aforementioned set is disallowed

Thankfully this is an easy fix. However, the "magic" of __ prefixed names will go away. That is, the following behavior will differ from v1.2:

(defrecord R [__a ___b])
(:__a (R. 42 9))
;=> 42

(deftype T [__a])
(.__a (T. 36))
;=> 36

(deftype T2 [a b __meta])
;; AssertionError

Any thoughts?

If anyone is interested, the changes reflected in the previous comment can be found on my own branch: https://github.com/fogus/clojure/tree/CLJ-837 Code-review would be nice.

differing behavior of undocumented features is a non-problem

Got it. Thanks Rich.

Added patch file.

I suggest that the documentation should simply say that all fields beginning with double-underscore (__) are reserved. You don't have to enforce that (as the patch allows any field name except __meta and __extmap), but it's better to be conservative with the documentation so that you have some room to innovate.

Just by inspection, it looks like patch has an extra space before the 'b' in the line near the end:

+ 2 (:___ b r)

0837-fixed is same as original patch, but with fixed unit test.

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.

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.

Converted from http://www.assembla.com/spaces/clojure/tickets/389
Attachments:
slurp-buffer-size.patch - https://www.assembla.com/spaces/clojure/documents/c7INhmGfSr34YDeJe5cbCb/download/c7INhmGfSr34YDeJe5cbCb

stu said: Updating tickets (#389, #371)

aaron said: Patch does not apply. It is not in the proper format. Please take a look at http://clojure.org/patches and resubmit. I will continue to review the code itself against the current master.

Contacted original poster about this to inform him of the move to JIRA and ensure interest in this patch

The patch has been reformatted and submitted.

Maybe duplicate: CLJ-668

CLJ-668 is a duplicate of this ticket. I have tested your patch and would like to consider it as a resolution to this ticket along with closing CLJ-668 as a duplicate. Can you please submit a patch to this ticket instead of a link to github? I am going to test all of the patches presented on OSX, Linux, EC2, and Windows.

Seems like the changes are already applied by your previous commit:

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

Just a leftover StringBuilder Object (patch attached)

Yes, that was a mistake. It should not have gone in under #441. I was doing some testing and somehow I didn't properly revert what I was working on. Please consider that a mistake for now and include a patch here so you can get credit for your work.

Thanks. Patch enclosed.

Thanks. I am going to be testing the various ideas on multiple platforms to see which (if any) is the best fit.

This patch carries a performance hit. For example:

Clojure 1.4.0

With Patch

This was tested with

(time (dotimes [_ 10] (slurp "file")))

with each execution of this form done 20 times.

Converted from http://www.assembla.com/spaces/clojure/tickets/369
Attachments:
0001-Check-interface-methods-for.patch - https://www.assembla.com/spaces/clojure/documents/b8JJfgBByr374seJe5cbCb/download/b8JJfgBByr374seJe5cbCb

richhickey said: We might have some better method name validity checking somewhere (i.e. not just '-', although that is probably most common)

Screened.

Without this patch, gen-interface can create methods with invalid names.

attached patch to "document that (flatten nil) returns an empty sequence"

1.3.0-beta1

patch to not register vars during macro expansion

patch still applies cleanly and all tests pass

tests pass and approach seems sound

This patch is the correct format; using `git format-patch`, instead of dumping the output of `git diff` to a file. Sorry about that.

Trivial docstring fix. Screened.