Welcome! Please see the About page for a little more info on how this works.

0 votes
in Clojure by

(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))


Generates the following warnings:


recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b


This is interesting for several reasons.  For one, if the arg to {{recur}} is a {{let}} form, there is no warning:


(fn [] (loop [b 0] (recur (let [a 1] a))))


Also, the compiler appears to understand the return type of {{loop}} forms just fine:


(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}


The problem can of course be worked around using an explicit cast on the {{loop}} form:


(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))


Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

*See Also:* CLJ-1422

*Patch:* 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch

47 Answers

0 votes
by

Comment made by: michaelblume

A naive attempt to bring the patch up to date results in

`
compile-clojure:

 [java] Exception in thread "main" java.lang.ExceptionInInitializerError
 [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
 [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
 [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
 [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
 [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
 [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
 [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
 [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7112)
 [java] 	at clojure.lang.Compiler.eval(Compiler.java:7416)
 [java] 	at clojure.lang.Compiler.load(Compiler.java:7859)
 [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
 [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
 [java] 	at clojure.lang.RT.load(RT.java:453)
 [java] 	at clojure.lang.RT.load(RT.java:419)
 [java] 	at clojure.lang.RT.doInit(RT.java:461)
 [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
 [java] 	... 1 more
 [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
 [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
 [java] 	... 17 more

`

0 votes
by

Comment made by: hiredman

the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)

0 votes
by

Comment made by: michaelblume

I get the same error I shared before applying the new patch to 1d5237f

0 votes
by

Comment made by: alexmiller

I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:

`
compile-clojure:

 [java] Exception in thread "main" java.lang.ExceptionInInitializerError
 [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
 [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
 ...
 [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
 [java] 	... 1 more
 [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
 [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
 [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
 [java] 	... 17 more

`

0 votes
by

Comment made by: bronsa

Looks like direct linking interacts with the diffs in this patch in non trivial ways.

0 votes
by

Comment made by: hiredman

I must have screwed up running the tests some how, I definitely get the same error now

0 votes
by

Comment made by: hiredman

after you get past the cast issuing (just adding some conditional logic there)

it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).

if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.

0 votes
by

Comment made by: hiredman

hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff

the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.

again I haven't done the extended testing with this patch.

here is an example of what the hoisted methods look like

0 votes
by

Comment made by: hiredman

there is still an issue with patch 7, (defn f (link: ^long y) (let (link: x (try (+ 1 0) (catch Throwable t y))) x)) causes a verifier error

0 votes
by

Comment made by: bronsa

Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:

  • fixes the bug mentioned in the last comment by (link: ~hiredman)
  • removes unnecessary whitespace and indentation changes
  • conforms indentation with the surrounding lines
  • squashes the commits into one as preferred by Rich

Authorship is maintained for (link: ~hiredman)

0 votes
by

Comment made by: hiredman

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.

0 votes
by

Comment made by: bronsa

Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter -- I just use whatever git format patch outputs.

One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame

0 votes
by

Comment made by: jafingerhut

Just now I added a suggestion to http://dev.clojure.org/display/community/Developing Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.

Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.

Only preference on patch file names is the one on that page -- that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.

0 votes
by

Comment made by: bronsa

0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces

0 votes
by

Comment made by: michaelblume

I have a verify error on the latest patch, will attempt to provide a small test case:

Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object;) Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)

...