<< Back to previous view

[TRDR-8] Fix for parsing of tagged literals Created: 12/Sep/13  Updated: 13/Sep/13  Resolved: 13/Sep/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: bug, patch

Attachments: Text File 0001-Fix-for-ctor-reading.patch     Text File reader.patch    
Patch: Code
Approval: Ok

 Description   

Parsing of tagged literals fails at the moment, e.g.

user> (require '[clojure.tools.reader :as r] :reload) (r/read-string "#java.lang.String[\"Hi\"]")
nil
ArrayIndexOutOfBoundsException 15 clojure.lang.RT.aget (RT.java:2239)

Attached patch corrects this problem, and brings the clojure logic in line with what's in LispReader.java



 Comments   
Comment by Nicola Mometto [ 13/Sep/13 7:18 AM ]

Thanks for the patch, can you post a patch that is the result of git format-patch so that we can keep your authorship on the commit?
(apply the patch, git add <path/to/reader.clj>, git commit -m "Fix ctor reading", git format-patch origin/master)

Also, can you please put the result of (count entries) in the let so that we compute it only once?

Thanks

Comment by Alex Coventry [ 13/Sep/13 12:06 PM ]

No worries, Nicola. I've attached a revised patch.

Best regards,
Alex

Comment by Nicola Mometto [ 13/Sep/13 12:38 PM ]

fixed https://github.com/clojure/tools.reader/commit/d9374f90448a4ff52ad83a2b75be2fa520a24db8





[TNS-12] Duplicate jar-file? definition in find.clj Created: 16/Sep/13  Updated: 10/Jan/14  Resolved: 17/Sep/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Alex Coventry Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File 0001-Remove-duplicate-of-jar-file-fn.patch    
Patch: Code

 Description   

There are two copies of the jar-file? function in find.clj.



 Comments   
Comment by Stuart Sierra [ 17/Sep/13 7:24 AM ]

Fixed in commit 0e4fe44be8ded594d0374d4cc21d6f42a697c18e

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TMACRO-2] protect let-bound symbols from macrolet expansion Created: 26/Nov/12  Updated: 27/Nov/12  Resolved: 27/Nov/12

Status: Resolved
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Tom Jack Assignee: Konrad Hinsen
Resolution: Completed Votes: 1
Labels: bug, patch

Attachments: Text File 0001-Don-t-apply-macro-expansion-to-protected-forms.patch     File macrolet-protect.diff    
Patch: Code and Test

 Description   

As discussed here: https://groups.google.com/d/msg/clojure-dev/-/UheAzkyI_WcJ

Patch macrolet-protect.diff fixes the issue and provides a test.



 Comments   
Comment by Tom Jack [ 26/Nov/12 2:55 PM ]

D'oh, patch macrolet-protect.diff has a bug — it doesn't macroexpand-1 if the symbol is protected.

Comment by Tassilo Horn [ 27/Nov/12 1:38 AM ]

This patch supersedes Tom's patch as discussed on the clojure-dev mailinglist. See

Message-ID: <CADygAw4ArNq4Z2=ZJmT6MwkBw160ShJmfQwoFEh4VOiwxfjDKQ@mail.gmail.com>

for reference.

The patch also adds letfn* as a form introducing protected symbols. That one was completely missing.

I added test cases for both let as well as letfn.

Comment by Konrad Hinsen [ 27/Nov/12 8:40 AM ]

Patch applied: https://github.com/clojure/tools.macro/commit/19c8197e10079f04e55d81c26884b9248762c2ca

Thanks!





[NREPL-50] Configurable eval function Created: 10/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

Status: Closed
Project: tools.nrepl
Component/s: None
Affects Version/s: None
Fix Version/s: 0.2.4

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Chas Emerick
Resolution: Completed Votes: 1
Labels: patch, patch,

Attachments: Text File 0001-NREPL-50-Overridable-eval-function.patch    
Patch: Code

 Description   

#'clojure.main/repl has an :eval keyword option that is currently not provided by the interruptable-eval middleware.

I'd like to be able to supply that argument on a per session basis.



 Comments   
Comment by Brandon Bloom [ 10/Apr/14 7:36 PM ]

Attaching patch to support :eval option on (= op :eval) messages.

Comment by Chas Emerick [ 11/Apr/14 7:20 AM ]

This patch adds that option on a per-evaluation basis, which I don't think you really want. Having this set on a per-session basis (as you originally described) makes more sense to me, esp. insofar as doing it that way makes it trivially controllable from "userland" (i.e. the human typing in expressions to be evaluated somewhere). Changing the shape of messages going back and forth requires tool support.

Comment by Brandon Bloom [ 11/Apr/14 9:44 AM ]

Ah, sorry, I should have explained why I ultimately did this at the per-message level. I've got a custom evaluation, but I want to be able to freely mix standard and custom evaluations during the same session. In my case, I have .clj files that use clojure.core/eval and .eclj files that use eclj.core/eval via vim-fireplace. As far as I can tell, Fireplace only opens one session per lein project, so session variables would have been a lot more work.

Is there some way that I can easily override session variables on a per message basis? If so, then I can switch to that to get both the user-level customization and tooling-level support. Otherwise, I'm open to suggestions.

Comment by Chas Emerick [ 12/Apr/14 12:40 PM ]

Sessions are essentially just the result of get-thread-bindings, plus some nREPL bookkeeping. So, as long as evaluate binds e.g. *eval*, users can just set! the var to change it. Though, unless you watch for that in your custom evaluation fn, you'll never be able to change it back. :-P This seems like a good enough reason to keep the option per-message, and require tools/users to ask for what they want each time.

(Tangentially, I just remembered that Piggieback does roughly the same thing you're looking for. I thought about pushing something like what you're suggesting down into nREPL, but I'm very cautious about changing it to suit my needs [myopia and so on]. Glad we have a second real use case, which makes me feel better about the whole thing.)

Comment by Brandon Bloom [ 13/Apr/14 12:09 PM ]

Yeah, set! seems like a surefire way to steamroll over later evaluations unless everybody does a defensive set! all the time, and even then, set! may not even be defined for the evaluator that the caller is expecting!

Maybe this means that there needs to be some sort of general-purpose way to override session variables per message? Then, we'd only need to define eval and all messages could be processed inside their own with-bindings call.

Comment by Chas Emerick [ 14/Apr/14 2:13 PM ]

Maybe, but that's a lot to bite off without having more than "wouldn't it be cool if…" as motivation.

Any thoughts as to whether the eval symbol's namespace should be implicitly {{require}}d?

Will be applying the proposed patch (maybe with a tweak or two) this week.

Comment by Brandon Bloom [ 14/Apr/14 2:23 PM ]

Clojure's standard load-file looks for .class or .clj files. EClj has to patch the loader to look for .eclj files too, and eclj.core may be re-loaded after the new evaluator is bootstrapped.

I have to pre-load the namespace manually either way. Therefore, I don't care if it implicitly requires the namespace, as long as it doesn't :reload the namespace.

Comment by Chas Emerick [ 18/Apr/14 10:04 AM ]

Committed, will be available in 0.2.4-SNAPSHOT momentarily.





[MTOWER-1] README should be updated to conform to contrib standard Created: 16/Sep/12  Updated: 17/Sep/12  Resolved: 17/Sep/12

Status: Resolved
Project: math.numeric-tower
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Christian Romney Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: documentation, patch

Attachments: Text File 0001-Improved-README-to-follow-standard-for-contrib.patch     Text File 0001-Improved-README-to-follow-standard-for-contrib.patch    
Patch: Code

 Description   

As per Sean Corfield's suggestion here: https://groups.google.com/forum/?fromgroups=#!searchin/clojure-dev/math/clojure-dev/p5oz42gR_sk/cesMHO9cDWEJ
the math.numeric-tower README.md should be updated to be more useful, especially to newbies.



 Comments   
Comment by Christian Romney [ 16/Sep/12 10:50 PM ]

Please ignore previous patch (doesn't format code examples correctly). This one looks better on Github. You can see it here: https://github.com/xmlblog/math.numeric-tower

Comment by Sean Corfield [ 17/Sep/12 11:33 AM ]

Patch applied.





[MCOMB-4] Performance enhancement for sorted-numbers? Created: 12/Jul/14  Updated: 19/Jul/14  Resolved: 19/Jul/14

Status: Resolved
Project: math.combinatorics
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Daniel Marjenburgh Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: enhancement, patch, performance

Attachments: File sorted-numbers-perf-enh.diff    
Patch: Code
Approval: Accepted

 Description   

Hi,

Came upon this as I was trying to improve performance. The implementation of sorted-numbers? (only used by permutations) is:
(defn- sorted-numbers?
"Returns true iff s is a sequence of numbers in non-decreasing order"
[s]
(and (every? number? s)
(every? (partial apply <=) (partition 2 1 s))))

<= and similar operators are variadic, so partitioning into pairs is unneeded.
(apply <= s) is a lot faster, but breaks for empty sequences, so an additional check is needed.

The implementation can be changed to:

(and (every? number? s)
(or (empty? s) (apply <= s)))

I benched this to be 10 to 15 times faster. A regression test with test.check was done to verify that the behaviour was not changed under any input.
A patch is also included.

Cheers,

Daniel



 Comments   
Comment by Andy Fingerhut [ 13/Jul/14 7:39 PM ]

Thanks for the submission. I don't know whether Mark would be interested in taking the test.check regression test you developed, too, but if you would be willing to share it as a patch, it might be considered for inclusion as well.

Comment by Daniel Marjenburgh [ 18/Jul/14 5:43 AM ]

Hm, I don't know how I could include the regression test as a patch. Because:
1) The current project does not use external libraries or Leiningen (yet). How do I add dependencies when not using Leiningen?
2) I tested the old implementation vs the new one with generated inputs. If the old implementation is gone, there's no test to add...

Comment by Mark Engelberg [ 19/Jul/14 5:06 PM ]

Since this function is only called once at the beginning of the permutations process, on a sequence that is usually 10 or fewer items, I can't imagine this improvement will have any meaningful impact on the overall running time of the permutations algorithm. Nevertheless, it's an improvement, so I've gone ahead and added it.





[MCOMB-1] math.combinatorics README should be updated to conform to contrib standard Created: 17/Sep/12  Updated: 17/Sep/12  Resolved: 17/Sep/12

Status: Resolved
Project: math.combinatorics
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Christian Romney Assignee: Mark Engelberg
Resolution: Completed Votes: 0
Labels: documentation, patch

Attachments: Text File 0001-Updated-README-to-conform-to-new-contrib-standard.patch    
Patch: Code

 Description   

As per Sean Corfield's suggestion here: https://groups.google.com/forum/?fromgroups=#!searchin/clojure-dev/math/clojure-dev/p5oz42gR_sk/cesMHO9cDWEJ
the math.combinatorics README.md should be updated to be more useful, especially to newbies.



 Comments   
Comment by Sean Corfield [ 17/Sep/12 11:35 AM ]

Patch applied.





[LOGIC-64] Support inequalities in finite domain sugar Created: 26/Oct/12  Updated: 28/Jul/13  Resolved: 27/Oct/12

Status: Closed
Project: core.logic
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File LOGIC-64-v1.patch    
Patch: Code

 Description   

The attached patch enables the follow code:

(run* [x]
  (infd x (interval 0 9))
  (eqfd (!= 6 (* 2 x))))

You can also substitute != with <, >, >=, or <=



 Comments   
Comment by David Nolen [ 27/Oct/12 11:33 AM ]

fixed, http://github.com/clojure/core.logic/commit/423639c62584c0100f295cc87c1cf2f721e986e3





[LOGIC-44] ex* could expand macros in patterns Created: 19/Jul/12  Updated: 17/Mar/13

Status: Open
Project: core.logic
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Joe Osborn Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: enhancement, patch, test

Attachments: Text File exstar-macros.patch    
Patch: Code and Test

 Description   

So, tagged data structures are probably interesting in a relational context. Say you have a relation with some default logic about dogs:

(defna friendlyo [Dog-Or-Breed]
    ([:Spot] succeed)
    ([:Spike] fail)
    ([Other-Dog] (fresh [Breed] (dog-breed Other-Dog Breed) (friendlyo Breed)))
    ([(breed :miniature-dachshund)] fail)
    ([(breed :golden-retriever)] succeed)
    ;. . .)

Assume there's a (defmacro breed [t] `[:breed ~t]).

That's nicer than having to drop [:breed :golden-retriever] in there or whatever, since it's compile-time-checkable, less error-prone, reduces duplication, etc.

This little patch makes ex* expand macros in patterns so it doesn't treat e.g. (breed :golden-retriever) as introducing a new LVar called "breed". Test also provided.



 Comments   
Comment by David Nolen [ 19/Jul/12 4:41 PM ]

I'm surprised that this doesn't already work. We have support for unifying expressions in the pattern already. Look at line 1230 in tests.clj in the master branch.

So this should just work, no need to explicitly support macros as far as I can tell. If it's not working, then there's a bug.

Comment by Joe Osborn [ 19/Jul/12 5:18 PM ]

At least on 0.7.5, matching against a macro gives a runtime error:

Exception in thread "main" java.lang.ClassCastException: clojure.core.logic.LVar cannot be cast to clojure.lang.IFn
	at rl.core$glyph_$fn__123$fn__144$fn__165$fn__166$_inc__167$fn__168.invoke(core.clj:61)
	at clojure.core.logic.Substitutions.bind(logic.clj:211)
	at rl.core$glyph_$fn__123$fn__144$fn__165$fn__166$_inc__167.invoke(core.clj:58)
	at clojure.core.logic$fn__1056$_inc__1057.invoke(logic.clj:1160)
	at clojure.core.logic$fn__1056$_inc__1057.invoke(logic.clj:1160)
	at clojure.core.logic$fn__898$_inc__899.invoke(logic.clj:823)
	at clojure.core.logic$fn__890$fn__891.invoke(logic.clj:828)

Using a fn instead of a macro gives the same:

Exception in thread "main" java.lang.ClassCastException: clojure.core.logic.LVar cannot be cast to clojure.lang.IFn
	at rl.core$drawable_$fn__235$fn__248$fn__249$_inc__250$fn__251.invoke(core.clj:67)
	at clojure.core.logic.Substitutions.bind(logic.clj:211)
	at rl.core$drawable_$fn__235$fn__248$fn__249$_inc__250.invoke(core.clj:65)
	at clojure.core.logic$fn__1056$_inc__1057.invoke(logic.clj:1160)
	at clojure.core.logic$fn__894$_inc__895.invoke(logic.clj:826)
	at clojure.core.logic$fn__1056$_inc__1057.invoke(logic.clj:1160)
	at clojure.core.logic$fn__898$_inc__899.invoke(logic.clj:823)
	at clojure.core.logic$fn__898$_inc__899.invoke(logic.clj:823)
	at clojure.core.logic$fn__898$_inc__899.invoke(logic.clj:823)
	at clojure.core.logic$fn__890$fn__891.invoke(logic.clj:828)

Here's (glyph-) for reference (don't mind all the extra [], I have a weird key/value thing because of some conveniences for maintaining fact identity in a temporal database):

(defna glyph- [Key Val]
	([[Thing] [Glyph]] (thing- [Thing]) (on-fire_ *turn* [Thing]) (== Glyph \δ))
	([[Thing] [Glyph]] (thing- [Thing]) (fresh [Type] (type- [Thing] [Type]) (glyph- [Type] [Glyph])))
	([[(type-enum :player)] [Glyph]] (== Glyph \@))
	([[(type-enum :dragon)] [Glyph]] (== Glyph \D))
	([[Type] [Glyph]] (== Glyph \?)))

and type-enum as a macro:

(defmacro type-enum [v] `[:enum :type ~v])

and as a fn:

(defn type-enum [v] [:enum :type ~v])

I'll mess around and see if my example works in HEAD.

Comment by Joe Osborn [ 19/Jul/12 5:37 PM ]

Same exception with this test case in HEAD (sorry for all the facts):

(defrel thing- [Thing])
(defrel type- [Thing] [Type])
(fact thing- [0])
(fact thing- [1])
(fact thing- [2])
(fact type- [0] [:player])
(fact type- [1] [:dragon])
(fact type- [2] [:pig])
(defn type-enum [t] [:type t])
(defna drawable- [Key]
  ([[Thing]] (thing- [Thing]) (fresh [Type] (type- [Thing] [Type]) (drawable- [Type])))
  ([[(type-enum :player)]] succeed)
  ([[(type-enum :dragon)]] succeed))

(deftest do-fns-work
	(is (= (run* [q] (drawable- [q])) '(0 1))))

Now that I look at it, I may be expecting a wrong-format return value, but the point is that I don't even get that far.

Using the REPL, I checked out how (defna drawable- . . .) expands (tidied up slightly):

(def drawable- (clojure.core/fn ([Key] 
  (clojure.core.logic/conda 
	  ((clojure.core.logic/fresh [Thing] (clojure.core.logic/== [Thing] Key) (thing- [Thing]) (fresh [Type] (type- [Thing] [Type]) (drawable- [Type])))) 
		((clojure.core.logic/fresh [type-enum] 
		  (clojure.core.logic/== [(type-enum :player)] Key) succeed))
		((clojure.core.logic/fresh [type-enum] 
		  (clojure.core.logic/== [(type-enum :dragon)] Key) succeed))))))

Note the (clojure.core.logic/fresh [type-enum] . . .) forms, which are exactly what I would not want to see in this case.

I'm not really sure why this doesn't work here yet works for the matche test case.

Comment by David Nolen [ 19/Jul/12 5:47 PM ]
[(type-enum :dragon)]

This pattern make it seem like you want to match:

[[:type :dragon]]

Note extra level of brackets here. Is this the case?

Even so I agree that the expansion doesn't look quite right. We should never descend into a seq form like that.

Comment by Joe Osborn [ 19/Jul/12 5:57 PM ]

Yes, that's exactly the desired outcome in this case--a tagged value in my naive interpretation. Is the reason it fails whereas the test on :1230 doesn't the fact that it's producing a vector and not a list? Changing the fn to return a list instead of a vector didn't seem to help.

My patch, fwiw, doesn't exhibit that behavior (at least for macros, haven't tested it with fns).

Comment by David Nolen [ 19/Jul/12 9:11 PM ]

What I mean is don't you want the following instead?

(defna drawable- [Key]
  ([[Thing]] (thing- [Thing]) (fresh [Type] (type- [Thing] [Type]) (drawable- [Type])))
  ([(type-enum :player)] succeed)
  ([(type-enum :dragon)] succeed))

Note that I removed a layer of square brackets.

Comment by Joe Osborn [ 20/Jul/12 10:28 AM ]

Nope! I actually want both. I'm doing some temporal logic stuff and I wanted some conveniences for "updating" a fluent, so I wanted to distinguish between the "key part" and the "value part" of the arguments. It looks silly for facts with no "value part", but it lets me write procedures and fns something like this:

(defrel heldo Time Fluent)
(defrel ¬heldo Time Fluent)
(declare fluent-obtainedo) ; most recent 'held' not terminated by a '¬held', or fail
(defn alter-fluent [Time Rel Key NewVal]
  ;todo: error check, ensure old != new, old obtains, new does not obtain
  (doseq [old-val (run* [old-val] (fluent-obtainedo Time [Rel Key old-val]))]
    (fact ¬heldo Time [Rel Key old-val]))
  (fact heldo Time [Rel Key NewVal]))
. . .
(fact heldo 0 ['pos [0] [0 0]])
. . .
(alter-fluent 1 'pos [0] [1 1])

And I write all the non-temporal fluents that way too for consistency and to help prevent mistakes.

Comment by David Nolen [ 20/Jul/12 2:58 PM ]

I'll try to give a closer look at this issue over the weekend.

Comment by David Nolen [ 17/Mar/13 7:05 PM ]

We're thinking about a more general solution here: http://github.com/clojure/core.logic/wiki/Better-syntax-support





[JDBC-96] execute! should accept a PreparedStatement Created: 13/May/14  Updated: 18/May/14  Resolved: 18/May/14

Status: Resolved
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Tejas Dinkar Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-execute-can-now-accept-prepared-statements-as-well.patch    
Patch: Code and Test

 Description   

This patch to clojure.java.jdbc allows you to pass in a prepared statement to the execute function.

I have already signed the CLA (Tejas Dinkar).

I can also open a pull request from here: https://github.com/gja/java.jdbc/tree/execute-should-accept-a-prepared-statement



 Comments   
Comment by Sean Corfield [ 14/May/14 12:26 AM ]

This looks very nice Tejas, thank you! I'll apply the patch at the weekend and verify everything and let you know if I hit any issues.

Comment by Sean Corfield [ 18/May/14 7:11 PM ]

Patch applied. Will be in 0.3.4 version of library.





[JDBC-59] Support Postgres in the test suite Created: 25/May/13  Updated: 26/Jul/13  Resolved: 26/May/13

Status: Resolved
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Dan Dillinger Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: patch, test

Attachments: File postgres-test-support.diff    

 Description   

The README for c.j.j currently indicates postgres support with instructions on how to run the tests against it; however, the tests fail if you do. There are two reasons:

  • float-or-double needs a postgres case or it fails because of equality classes
  • the postgresql jdbc driver's .getGeneratedKeys() implementation returns all columns in the row, not just the auto-generated keys.

A patch is attached: postgres-test-support.diff (05/25/2013)



 Comments   
Comment by Sean Corfield [ 26/May/13 10:19 AM ]

Applied patch. Thanx!





[DXML-25] Emit Empty Elements using EmptyElementTag Created: 09/Jul/14  Updated: 09/Jul/14

Status: Open
Project: data.xml
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alexander Kiel Assignee: Ryan Senior
Resolution: Unresolved Votes: 0
Labels: enhancement, patch
Environment:

Does not apply


Patch: Code and Test

 Description   

Currently data.xml emits empty elements (elements without content) using start and end tags. The XML spec also allows special empty tags like <foo/>.

I need to serialize XML using such special empty tags because a device, I want to communicate with, does require empty tags. The device is just not able to parse XML messages using start and end tags.

I created a branch on GitHub where I implemented empty tags in the emit function. I'm not familiar how to create a patch. So for now here is the link to the compare view.

As I wrote in my commit message we should discuss, whether a option to the emit function would be a better solution.






[DJSON-16] Add positional tracking to JSON reader Created: 18/May/14  Updated: 06/Jun/14  Resolved: 06/Jun/14

Status: Closed
Project: data.json
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Tim Clemons Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: enhancement, errormsgs, patch

Attachments: Text File 0001-Explicity-define-whitespace-characters.patch     Text File 0002-Add-position-tracking-to-reader.patch     Text File 0003-Add-line-and-column-information-to-read-exceptions.patch     Text File 0004-Add-stack-of-structure-starting-points-to-reader.patch     Text File 0005-Add-track-pos-argument-to-read.patch     Text File 0006-Replace-instances-of-printf-with-format.patch    
Patch: Code and Test

 Description   

The attached patches add an optional argument to clojure.data.json/read, :track-pos?, that causes the line and column number information for each array and object member to be stored as metadata on the result. Line and column numbers are also added to the various exception messages. Useful for doing validation on a JSON file so that the user can mor easily determine where a problem exists.



 Comments   
Comment by Tim Clemons [ 27/May/14 4:37 PM ]

FYI, it appears my Contributor Agreement has been received and processed: http://clojure.org/contributing

Comment by Stuart Sierra [ 06/Jun/14 3:24 PM ]

I'm not opposed to this feature in principle, but I do not want to take the performance hit of this patch: more than 5X slower in my tests, regardless of whether or not you use the feature.





[DGEN-2] string generator function comment is inaccurate Created: 26/Aug/14  Updated: 26/Aug/14

Status: Open
Project: data.generators
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Ross McDonald Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: File fix-string-generator-comment.diff    
Patch: Code

 Description   

comment currently says "... chars from v ..."

"v" needs to become "f"

this is addressed in the attached patch 'fix-string-generator-comment.diff'






[CTYP-92] Implement a defn> macro Created: 06/Nov/13  Updated: 08/Nov/13  Resolved: 08/Nov/13

Status: Closed
Project: core.typed
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Reid McKenzie Assignee: Ambrose Bonnaire-Sergeant
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File typed.patch    
Patch: Code and Test

 Description   

Implement a defn> macro which wraps fn> and def, providing concise function declaration and annotation.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 08/Nov/13 9:54 AM ]

Merged, included in 0.2.17.





[CMEMOIZE-6] assert on ttl fn value makes any callable other than a pure clojure.lang.Fn be rejected Created: 27/Jun/13  Updated: 28/Jun/13  Resolved: 28/Jun/13

Status: Resolved
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Max Penet Assignee: Fogus
Resolution: Completed Votes: 0
Labels: bug, patch

Attachments: Text File fix-assert.patch    
Patch: Code

 Description   

ttl memoize used to accept multimethods for instance, this is no longer the case since the latest version where an assert? was introduced that checks using fn?

The patch attached allows any of the following: clojure.lang.IFn, clojure.lang.AFn, java.lang.Runnable, java.util.concurrent.Callable.

(I am a registered contributor listed under "Maximilien Penet (mpenet)")



 Comments   
Comment by Fogus [ 28/Jun/13 7:17 AM ]

This is fixed on master. I will push a point-release out to Maven Central later today.

Comment by Fogus [ 28/Jun/13 7:31 AM ]

Applied to master. A new release will come soon.





[CLJS-861] In Clojurescript IE8 cljs.reader can't read numbers Created: 18/Sep/14  Updated: 20/Sep/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Zubair Quraishi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch
Environment:

IE8 Web Browser (used in my company)


Attachments: PDF File Clojure_CA.pdf     File reader.cljs    
Patch: Code

 Description   

This change is made because IE8 always returns 0 when cljs.reader reads an integer.

This is caused because the matching pattern in most browsers returns nil for unmatched patterns but IE8 returns "" instead of nil



 Comments   
Comment by Zubair Quraishi [ 18/Sep/14 10:33 PM ]

https://github.com/clojure/clojurescript/pull/39#issuecomment-56069471

https://github.com/zubairq/clojurescript/commit/7ec0144c53b47a831f8696f9eaf97cbcfe06b2e7

https://github.com/JulianBirch/cljs-ajax/issues/46

Comment by David Nolen [ 19/Sep/14 11:25 AM ]

Please e-sign the CA here: http://clojure.org/contributing, thanks!

Comment by Zubair Quraishi [ 19/Sep/14 12:14 PM ]

Ok, I e-signed it and I got back a PDF. Do I have to send the PDF or attach it anywhere?

Comment by David Nolen [ 19/Sep/14 3:16 PM ]

You are now listed as a contributor. I need a properly formatted patch please follow these instructions: https://github.com/clojure/clojurescript/wiki/Patches. Thanks.

Comment by Zubair Quraishi [ 20/Sep/14 6:26 AM ]

I'm not sure what I have to do different from the patch I already submitted? I made a branch, ie8-fix-cljs-reader and made the change to:

https://github.com/zubairq/clojurescript/blob/ie8-fix-cljs-reader/src/cljs/cljs/reader.cljs

https://github.com/zubairq/clojurescript/commit/7ec0144c53b47a831f8696f9eaf97cbcfe06b2e7

I used "CLJS-NNN: TICKET TITLE" as the title for the ticket, "CLJS-854: Fix for IE8 cljs.reader to read integers correctly"

and then I did "git format-patch master --stdout > cljs_ticket_number.patch" before I pushed the change, with "git format-patch master --stdout > cljs_CLJS-854.patch".

Am I missing a step or have a made a mistake, or do I just need to do the same thing again?





[CLJS-661] Support a try/catch-all mechanism Created: 04/Nov/13  Updated: 05/Nov/13  Resolved: 04/Nov/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-661-v001.patch    
Patch: Code and Test

 Description   

See http://dev.clojure.org/display/design/Platform+Errors



 Comments   
Comment by David Nolen [ 04/Nov/13 10:45 PM ]

fixed, https://github.com/clojure/clojurescript/commit/9feed772a6db1d2697de387f14c05e7e99c9d891

Comment by Brandon Bloom [ 05/Nov/13 9:32 AM ]

See also http://dev.clojure.org/jira/browse/CLJ-1293





[CLJS-615] Warning when required lib does not exist on disk Created: 08/Oct/13  Updated: 22/Feb/14  Resolved: 17/Feb/14

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Timothy Pratley
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File CLJS-615.patch    
Patch: Code and Test

 Description   

Currently when analyzing dependencies in analyzer.clj we do not warn when the library does not exist on disk. The error should be similar to the one emitted by Clojure so that the user is aware how the namespace maps to directories and filenames.

Problem
==

hello.cljs:

(ns hello
(:require [typomina :refer [x]]))

Expected: Should be reported when typomina is a namespace never provided.
Actual: when optimization is none or whitespace, no warnings are given.
Expected: when x does not exist in typomina, hello should not compile
Actual: x is not verified even if typomina exists

Background
==

Dependencies are not resolved at cljs compile time.
cljs.closure docstring:
"build = compile -> add-dependencies -> optimize -> output"

Dependencies are deferred to Closure
https://github.com/clojure/clojurescript/wiki/Differences-from-Clojure:
"designed to leverage Google's Closure library, and participates in its dependency/require/provide mechanism"

Closure compiler detects missing dependencies when optimizations simple or advanced are applied:
./bin/cljsc hello.cljs '{:optimizations :simple :output-to "hello.js"}'
Dec 31, 2013 4:10:03 PM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: hello:3: ERROR - required "typomina" namespace never provided
goog.require('typomina');
^
ERROR: JSC_MISSING_PROVIDE_ERROR. required "typomina" namespace never provided at hello line 3 : 0

But not for whitespace or none. For none, Closure is not even called by cljs "optimize".
./bin/cljsc hello.cljs '{:optimizations :whitespace :output-to "hello.js"}'

The compile phase of Clojurescript looks for things to compile by accumulating anything it finds in :import :require :use :require-macros :use-macros. However if it can't find those dependencies it is not considered an error and this is important to preserve because those dependencies can be provided by JavaScript. Clojurescript makes no distinction between import and require.

Solution
==

(1) If there are unprovided dependencies after get-dependencies, let the user know:
closure.clj:add-dependencies
provided (mapcat :provides (concat required-js required-cljs))
unprovided (clojure.set/difference (set requires) (set provided))] (when unprovided
(apply println "ERROR, required namespace never provided for" (sort unprovided)))

(2) Build the JavaScript index prior to cljs compilation, and use it to determine if a require is not provided by either JS or CLJS
analyzer.clj:analyze-deps

(3) A useful warning for :undeclared-ns-form was hidden due to typos in the 'type' checking. 'type' checking outside of (warning ) was redundant as the (warning ) handler checks if that 'type' is enabled. External checks removed.

(4) no-warn was used for subdeps in analyze-deps
It seems this will just hide real issues.

(5) There was an opts flag :warnings which would disable :undeclared-var :undeclared-ns :undeclared-ns-form
when undefined. Tools like leincljsbuild do not enable it, and I can't find the option documented or recommended. This patch removes the :warnings option in favor of reporting them. Obviously this means you see more warnings reported now. They are real warnings. I recommend a separate ticket/patch to expose and document warning controlling options (:warnings should control all warnings, not just undeclared - and there should be the ability to turn on/off individual or sets of warnings.) Also (repl) function takes a "warn-on-undeclared" optional key which if missing will disable error checking - I think I should take this out or default it to true when not present.

Some discussion here:
https://groups.google.com/forum/#!topic/clojurescript/7vaEETMW5xg



 Comments   
Comment by David Nolen [ 02/Jan/14 1:00 PM ]

This looks nice and comprehensive, but it will take some time for me to review. Will try to give it a look over the weekend.

Comment by Timothy Pratley [ 03/Jan/14 3:40 PM ]

O.K. great
Some things to consider:

a) Many more warnings are displayed now. To me that is a very good thing. It might be a shock to some users with existing codebases. Things like:
(JSON/stringify x) [should be (js/JSON.stringify x)]
canvas/offsetWidth [should be (.-offsetWidth canvas)]

b) Warnings are propagated from libraries e.g.:
WARNING: clone already refers to: /clone being replaced by: domina/clone at line 147
Again to me this is a very good thing, but it might shock users.

c) Behavior of (1) does not work well with cljsbuild. It is fine for a clean build, but with multiple dependencies and only touching one file, cljsbuild only recompiled that particular file and reports the other dependencies as missing. I plan to investigate further. We don't really need to post check dependencies provided (2) is acceptable and could just remove (1).

I think we will need to discuss further on turning on/off warnings, how that should be exposed to users, and what the default behavior should be.

Comment by Timothy Pratley [ 03/Jan/14 10:24 PM ]

building a directory results in spurious require warnings - investigating further

Comment by Timothy Pratley [ 05/Jan/14 2:42 AM ]

Additional changes to address directory build warnings

Comment by David Nolen [ 07/Jan/14 7:11 AM ]

Is CLJS-740 also addressed by this patch?

Comment by Timothy Pratley [ 07/Jan/14 10:27 AM ]

Yes, that is correct.

Comment by Timothy Pratley [ 15/Jan/14 10:58 AM ]

Hi David, have you had a chance to look at the patch? Let me know if it requires any adjustments.

Comment by David Nolen [ 15/Jan/14 11:47 AM ]

I will review but honestly what we need are some testers besides myself.

Comment by David Nolen [ 15/Jan/14 1:06 PM ]

Ok I reviewed the patch. It looks good however I cannot apply it with `git am`.

Comment by David Nolen [ 17/Jan/14 9:21 AM ]

The problem with the patch is that's not plain text. I copied to a file on a machine to fix that. I applied it and I now get a series of warnings when running the tests. These need to be addressed by the patch. In addition when I run the ClojureScript REPL I get many many errors.

Comment by David Nolen [ 17/Jan/14 9:26 AM ]

I've gone and applied CLJS-740 to master as it's a simpler fix for an immediate issue. Let's rebase this one and address the issues. Thanks!

Comment by Timothy Pratley [ 03/Feb/14 12:39 AM ]

Attaching rebased patch which resolves tests emitting warnings and repl not launching.

Comment by David Nolen [ 03/Feb/14 8:22 AM ]

With the patch applied running the tests still emit warnings for goog.string, the constants table, and line 1956 in the core_test.cljs. These need to be addressed.

Comment by Timothy Pratley [ 16/Feb/14 2:20 PM ]

./scripts/test warnings suppressed. Not sure why this patch doesn't have the previous patch included in it, presumably I need to do something to combine them.

Comment by David Nolen [ 16/Feb/14 2:43 PM ]

High level question - what is the logic for the suppression of the warnings? Also do I need to apply two patches?

Comment by Timothy Pratley [ 16/Feb/14 5:33 PM ]

Hi David, the warnings around goog.string were suppressed by requiring goog.string in test_core. This is necessary because the with-out-str macro expands to goog.string/stuff. In one sense this seems correct in that code relying on good.string should require it, but in another sense feels bad because using core should not require other stuff. Alternatively if there is a list of things we know we need like goog.string perhaps it should just be treated as known. In fact for the constants table dependency I have to treat it as known because it is compiler internal. For the line 1956 there was a self-referential def form, and so the warning seemed appropriate. To silence that one I added an empty def before the self-referential def.

I think the reason why there are two patches is I must have applied the original to my master to check it, I'll post a fresh one soon as I sort it out.

Comment by David Nolen [ 16/Feb/14 5:39 PM ]

Ok, I think the preferred solution is to make any Google Closure we import/require in core.cljs to be implicit so we don't have to do this in test_core.cljs as you've done.

Comment by Timothy Pratley [ 16/Feb/14 5:44 PM ]

Single file patch

Comment by Timothy Pratley [ 16/Feb/14 5:46 PM ]

Ok sure no problem will change that

Comment by Timothy Pratley [ 17/Feb/14 1:17 AM ]

Added goog.string to the special list in confirm-ns that do not need to be required, as with-out-str macro expands to that.

Comment by David Nolen [ 17/Feb/14 8:24 AM ]

Patch looks good but could we please get a squashed patch? Thanks.

Comment by Timothy Pratley [ 17/Feb/14 11:24 PM ]

Squashed patch attached

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/e16a3dae7a63d1516f30cbeb065afeef6fbfb484

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

Awesome patch many thanks!

Comment by Chas Emerick [ 18/Feb/14 7:44 AM ]

The addition of :js-dependency-index to the compiler environment raises a minor complication that I'd like to address; see CLJS-770.

Comment by Gary Trakhman [ 22/Feb/14 12:20 AM ]

I made a note on github, and I'll repeat it here.

The patch in question causes errors when the rhino repl is initialized:

I saw the behavior first by using piggieback, then verified it with the test that was disabled in this commit:
https://github.com/clojure/clojurescript/commit/1b3e1c6168cba66c12749f5bcb6747ef0b8a2950

Reverting the patch in this ticket fixed the problem.

Comment by Gary Trakhman [ 22/Feb/14 12:31 AM ]

I also verified that reverting my patch from #764 has no effect.

Comment by Chas Emerick [ 22/Feb/14 8:17 AM ]

Gary, what are the errors you see? I know Austin won't work because of what's described in CLJS-770, but perhaps there's more going on?

Comment by Gary Trakhman [ 22/Feb/14 4:00 PM ]

the workaround in CLJS-770 seems to fix the problem I'm seeing.

Made my test do this, and it passes:
(doto (env/default-compiler-env)
(swap! assoc :js-dependency-index (cljs.closure/js-dependency-index {})))





[CLJS-575] cljsc.bat emit FileNotFoundException when compile samples in windows Created: 25/Aug/13  Updated: 19/Jun/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Park Sang Kyu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, bug, patch
Environment:

in windows 7


Attachments: File cljsc.bat.diff     File cljsc-path.bat    
Patch: Code

 Description   

cljsc.bat emit FileNotFoundException when it compile samples of the ClojureScript project in windows like below.

------------------------------------------------
Exception in thread "main" java.io.FileNotFoundException: Could not locate cljs/closure__init.class
or cljs/closure.clj on classpath:
------------------------------------------------

It is caused by lack of a backslash in the end of path of the system environment variable, %CLOJURESCRIPT_HOME% set by a user.
In the case CLASSPATH is set to "C:\\clojure\clojurescriptsrc\clj;C:\\clojure\clojurescriptsrc\cljs" and this make it impossible for javac to find cljs/clojure.clj file.

So it can be solved by adding a backslash to the path of %CLOJURESCRIPT_HOME%.

I attached the patched file, "cljsc-path.bat"



 Comments   
Comment by David Nolen [ 04/Sep/13 11:04 PM ]

Can we please get a proper git diff thanks (and please send in your CA)! Also would be nice to get Windows users to check this out.

Comment by Park Sang Kyu [ 15/Sep/13 3:16 AM ]

git diff

Comment by David Nolen [ 05/Oct/13 11:55 AM ]

Thank you! Have you sent in your CA? http://clojure.org/contributing

Comment by Park Sang Kyu [ 19/Jun/14 10:24 AM ]

Yes i have sent my CA.

Comment by David Nolen [ 19/Jun/14 10:27 AM ]

Excellent, the patch is not correctly formatted. Can we get a new patch that conforms to http://github.com/clojure/clojurescript/wiki/Patches





[CLJS-541] review inlining macros for subtle evaluation order bugs Created: 15/Jul/13  Updated: 27/Jul/13  Resolved: 16/Jul/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-541.patch    
Patch: Code and Test

 Description   

instance? suffers from this problem



 Comments   
Comment by David Nolen [ 16/Jul/13 6:12 AM ]

fixed http://github.com/clojure/clojurescript/commit/37b4ccefb085aac1cc30a90d13cd5bee736e00c1





[CLJS-485] clojure.string/replace ignores regex flags Created: 12/Mar/13  Updated: 19/Mar/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Esa Virtanen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, patch, test

Attachments: Text File 0001-Take-regex-flags-m-i-into-account-in-clojure.string-.patch    
Patch: Code and Test

 Description   

The replace function in namespace clojure.string ignores regex flag provided in the match pattern. For example:

CLJS
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am NOT matched"
CLJ
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am matched"

The attached patch fixes this by parsing the m and i flags, if set, from the match object, instead of explicitly setting only "g".



 Comments   
Comment by Chas Emerick [ 19/Mar/14 9:29 AM ]

I can confirm the bug. The attached patch applies cleanly, and works as expected.

Esa, sorry for the long delay (this one must have slipped through the cracks)! Could you please submit a contributor's agreement, so that your patch can be merged? More info is here:

http://clojure.org/contributing





[CLJS-441] Require implicit 'do nodes for all blocks in AST Created: 12/Dec/12  Updated: 27/Jul/13  Resolved: 21/Dec/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-441-v001.patch    
Patch: Code

 Description   

Discussion here: https://groups.google.com/d/topic/clojure-dev/6CVTsW1EQn4/discussion



 Comments   
Comment by Brandon Bloom [ 12/Dec/12 1:18 PM ]

This patch depends on the patch at http://dev.clojure.org/jira/browse/CLJS-440

Comment by Brandon Bloom [ 12/Dec/12 3:18 PM ]

This patch would also address http://dev.clojure.org/jira/browse/CLJS-416

Comment by David Nolen [ 21/Dec/12 5:36 PM ]

Fixed http://github.com/clojure/clojurescript/commit/764565e9e7696379ada5ac8168b20ee5f9cde6a2





[CLJS-440] Replace :is-loop flag in AST with distinct :loop op Created: 12/Dec/12  Updated: 27/Jul/13  Resolved: 21/Dec/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-1126-v001.patch     Text File CLJS-440-v001.patch    
Patch: Code

 Description   

Discussion here: https://groups.google.com/d/topic/clojure-dev/6CVTsW1EQn4/discussion



 Comments   
Comment by Brandon Bloom [ 12/Dec/12 1:14 PM ]

Whoops. Accidentally made a Clojure issue instead of a ClojureScript issue. Moved the ticket and reuploaded patch with correct ticket #

Comment by David Nolen [ 21/Dec/12 5:12 PM ]

fixed, http://github.com/clojure/clojurescript/commit/26b2541cd66d3139eca354434a56137218b17d09





[CLJS-437] Missing "Too few arguments to if" check Created: 06/Dec/12  Updated: 27/Jul/13  Resolved: 07/Dec/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-437-v001.patch    
Patch: Code

 Description   

(if) and (if 1) both return nil. JVM clojure throws "Too few arguments to if".



 Comments   
Comment by David Nolen [ 07/Dec/12 12:03 AM ]

fixed, https://github.com/clojure/clojurescript/commit/9abeb143f66ad6d92756c2dd702966f63ae76c27





[CLJS-432] Include line and file information in error messages Created: 28/Nov/12  Updated: 27/Jul/13  Resolved: 30/Nov/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-432-v001.patch     Text File CLJS-432-v002.patch     Text File CLJS-432-v003.patch     Text File CLJS-432-v004.patch     Text File CLJS-432-v005.patch     Text File CLJS-432-v006.patch    
Patch: Code

 Description   

Just as the ana/warning function did, now errors and assertions include line and file source information. I needed this sorely.



 Comments   
Comment by Brandon Bloom [ 28/Nov/12 6:13 PM ]

Added v2 of patch that will handle any exception during parsing. Similar could be done during code generation. This approach seems much more robust and less invasive.

Comment by Brandon Bloom [ 28/Nov/12 11:15 PM ]

That v2 was made hastily. Upon further thought, it seems broken in the face of recursive analysis. I think the right thing, if we prefer the exception catching approach, is to rethrow as a custom exception type, which would be allowed through un-re-wrapped.

For example:

(try
  (parse-form ...)
  (catch AnalysisError e
    throw e)
  (catch Throwable e
    (throw (AnalysisError. env e))))
Comment by David Nolen [ 29/Nov/12 11:32 AM ]

I don't have a problem with the approach in the first patch. I don't really see how a less invasive patch is even possible - you still need to pass the environment to some assertion check if you are going to throw a custom exception as well.

Comment by Brandon Bloom [ 29/Nov/12 2:28 PM ]

v3 of patch fixes issue with v2

Comment by Brandon Bloom [ 29/Nov/12 2:50 PM ]

v4 of patch as discussed in irc

Comment by Brandon Bloom [ 29/Nov/12 2:57 PM ]

v5 also catches error from parse-invoke

Comment by Brandon Bloom [ 29/Nov/12 5:05 PM ]

v6 improves errors in repl as discussed in IRC

Comment by David Nolen [ 30/Nov/12 11:25 AM ]

fixed, http://github.com/clojure/clojurescript/commit/af4ab91754d30f082b117b40c07ea94d0063c0d6





[CLJS-431] namespace and name fail on '/ Created: 27/Nov/12  Updated: 27/Jul/13  Resolved: 30/Nov/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-431-v001.patch    
Patch: Code and Test

 Description   

Both the 'namespace and the 'name functions use .lastIndexOf to search for "/". This fails on the division symbol if you don't provide a starting index to search from.



 Comments   
Comment by David Nolen [ 30/Nov/12 11:56 AM ]

fixed in commit 5ac15039c685af19810dc5b35f06a496be1f3369





[CLJS-426] subvec function not behaving consitently with invalid subranges Created: 22/Nov/12  Updated: 27/Jul/13  Resolved: 23/Nov/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Roman Gonzalez Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, patch
Environment:

Ubuntu precise 64 bits, Mac OS X Lion


Attachments: Text File cljs_subvec.patch     Text File cljs_subvec_revised1.patch     Text File cljs_subvec_revised.patch    
Patch: Code and Test

 Description   

When using the subvec function with a as a parameter vector and a range that is not in the original vector, the function returns a value:

(subvec [1 2 3] 0 4) => [1 2 3 nil]

However, when using with seqs, it works as it supposed to:

(subvec (range 3) 0 4) => ERROR: Index out of bounds

This is because the validation of ranges is not happening at build time of the subvec type, this bug contains a patch that adds a new private `build-subvec` function into cljs.core.



 Comments   
Comment by Roman Gonzalez [ 22/Nov/12 5:05 PM ]

Patch for bug

Comment by David Nolen [ 23/Nov/12 2:51 PM ]

This mostly looks good but there is a typo in the patch:

(build-subvec. meta (-assoc v v-pos val) ...
Comment by Roman Gonzalez [ 23/Nov/12 3:09 PM ]

Revised patch, removing small typo

Comment by Roman Gonzalez [ 23/Nov/12 3:18 PM ]

Removing useless ^:mutable meta on function parameter

Comment by David Nolen [ 23/Nov/12 3:25 PM ]

fixed, http://github.com/clojure/clojurescript/commit/ee25599abb214074cbeefe37b399038d70c6ab89





[CLJS-424] Internal representation of keywords can not be differentiated by printable characters Created: 19/Nov/12  Updated: 27/Jul/13  Resolved: 22/Dec/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-424-v1.patch    
Patch: Code

 Description   

Currently, Symbols are represented by (str \uFDD1 \' name) and keywords are represented by (str \uFDD0 \' name)

The \uFDDX character is not printable, so it appears as a little box when printed in a javascript repl such as the browser's console. The following character is always a \' regardless of whether the object is a symbol or a keyword. This has bitten me during debugging on several occasions. The attached patch changes keywords to be represented internally by (str \uFFD1 \: name)



 Comments   
Comment by David Nolen [ 22/Dec/12 3:45 PM ]

fixed, http://github.com/clojure/clojurescript/commit/21eab986826bd7a9e852712aaeda647a5de59b3b





[CLJS-416] emit-block doesn't use context argument Created: 09/Nov/12  Updated: 21/Dec/12  Resolved: 21/Dec/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-316-v1.patch    
Patch: Code

 Description   

emit-block currently has arguments [context statements ret] but the context argument is never used and all call sites always draw statements and ret from the same block AST node. The attach patch simplifies emit-block and all call sites to use arguments [{:keys [statement ret]}]



 Comments   
Comment by Brandon Bloom [ 21/Dec/12 5:30 PM ]

Obsoleted by http://dev.clojure.org/jira/browse/CLJS-441





[CLJS-409] Small code cleanup for emitting goog.provide statements Created: 27/Oct/12  Updated: 27/Jul/13  Resolved: 27/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-409-v1.patch    
Patch: Code

 Description   

Just abstracts out a duplicated piece of code.



 Comments   
Comment by David Nolen [ 27/Oct/12 4:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/d8f490e9b958a43e97b34808e76d977f29fee8c9





[CLJS-408] Include :form on fn :methods in AST Created: 24/Oct/12  Updated: 27/Jul/13  Resolved: 27/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-408-v1.patch    
Patch: Code

 Description   

All core CLJS AST nodes include a :form key. Several quasi-nodes similarly include their originating code forms. Although :fn nodes include their :form, it varies between single and multiple arity functions. I've found it easier to assume multiple arities and always analyze :methods directly. Unfortunately, the methods lack :form keys. This simple patch exposes the form of each method, simplifying function transformations.



 Comments   
Comment by David Nolen [ 27/Oct/12 4:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/3ea4a9309346a2a2d0983dc535b9b00531bfc90f





[CLJS-398] new macro cljs.core/extend-instance Created: 19/Oct/12  Updated: 27/Jul/13  Resolved: 19/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-new-macro-clojure.core-extend-instance-can-be-used-t.patch     Text File 0001-Test-for-extend-instance.patch    
Patch: Code and Test

 Description   

Patch introduces an extend-instance macro, similar to extend-type.
It doesn't extend the .prototype property, but assigns the prototype impl members to the instance itself.

This is useful to let existing instances, e.g. parsed JSON directly implement a protocol.



 Comments   
Comment by David Nolen [ 19/Oct/12 5:13 PM ]

Rich Hickey already had a good name for an operation like this - specify.

Comment by Herwig Hochleitner [ 19/Oct/12 6:10 PM ]

OK, I've declined the ticket, since not only the name but also the patches should be disregarded.

The problem with above impl is that the generated implementing fns get instantiated, every time extend-instance is evaluated. That is not acceptable on a per-instance basis.

I will work on a new macro called specify, which allows passing implementing fns.

Is a syntax similar to clojure.core/extend right for specify?
Should I create a new ticket?

Comment by David Nolen [ 19/Oct/12 7:51 PM ]

As far as I understand it the interface for specify should be the same as extend-type.





[CLJS-397] Omit var reads in statement context Created: 19/Oct/12  Updated: 27/Jul/13  Resolved: 23/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch, size

Attachments: Text File 0001-CLJS-397-var-reads-in-a-statement-context-get-omitte.patch    
Patch: Code

 Description   

Attached patch updates cljs emitter to not emit var references in statement context.
That causes toplevel deftypes to not return their generated type, which lets gclosure strip them if unused

cljs helloworld shrinks from ~90K to ~69K

https://groups.google.com/d/topic/clojure/LNfJRw07u8I/discussion



 Comments   
Comment by David Nolen [ 19/Oct/12 5:15 PM ]

Can we get the ticket # in the commit message? Thanks!

Comment by Herwig Hochleitner [ 19/Oct/12 5:42 PM ]

Of course, sorry! Here is a new patch, if you please.

Comment by David Nolen [ 19/Oct/12 7:32 PM ]

Thanks. I'm confused about the second aspect of the patch, what do you mean by bogus error messages?

Comment by Herwig Hochleitner [ 19/Oct/12 11:23 PM ]

Well, the repl uses a :statement context to generate the javascript which gets printed out as part of a stack trace. When setting :expr back to :statement in the repl, you can try the following scenario:

  • enter into the repl (def val :worked) (if (> (Math/rand) 0.5) val (throw ""))
  • half of the time you will get :worked
  • half of the time you will get a stacktrace citing 'ns.val = ":worked"; if (Math.rand() > 0.5){}{throw ""}'
  • that makes no sense

With the full patch, the printout will be smth like '(function(){ns.val = ":worked"; if (Math.rand() > 0.5){return ns.val}{throw ""}})()', which is noisier, but more like what actually got evaluated by the repl client.

##EDIT#APPEND##

The reason the repl still works, even when analyzing in a statement context with the patch is, that in order to get the return value, the repl statement has to be evaluated in an :expr context anyway.
This happens by unquoting it into a wrapper form. With the patch the wrapper form also gets analyzed in an :expr context, which doesn't hurt.
Only when an exception gets caught, the js of the unwrapped expr is used, which now always matches the its counterpart in the wrapper form.

Comment by David Nolen [ 20/Oct/12 9:33 AM ]

I'm still confused. Does this additional modification have anything to do with the ticket? By that I mean does the patch require the second modification? If it does can you explain a further? Thanks.

Comment by Herwig Hochleitner [ 20/Oct/12 1:17 PM ]

The patch doesn't require the second modification in the sense that everything still works if it's left out.

The patch requires the modification in the sense that it would break stack traces on the REPL otherwise + the two changes should be reverted together if and when we move such optimizations out of the emitter into an optimization pass.

So the trade off is in always having the correct code in a REPL stacktrace in exchange for making it more verbose. Again, this doesn't influence behavior, so it's a matter of prioritizing requirements.

Comment by David Nolen [ 21/Oct/12 3:48 PM ]

I still don't understand why/how the part of the patch that addresses the ticket breaks stack traces. Can you explain?

Comment by Herwig Hochleitner [ 22/Oct/12 10:06 AM ]

1) var read statements get omitted
2) typing "var" into the repl still works, because it's analyzed in a wrapper form
3) if it throws, the printed generated js is the original form in a statement context, you can observe this with above code sample.

Comment by David Nolen [ 22/Oct/12 10:16 AM ]

Ok I understand now, I don't think the second part of the patch is relevant. The user entered two statements, not one combined in an implicit do which is what the other part of the patch does. Can we get a new patch that only includes the part that addresses the ticket? Thanks!

Comment by Herwig Hochleitner [ 22/Oct/12 2:58 PM ]

For the record: There was some talk in the chat, where I laid down my rationale for changing the repl specifically: "Technically repl inputs have to be in an expr context, to capture the return val. And they are, by virtue of the wrapping form. The change makes that fact explicit, thereby keeping stack traces sane."

I also discovered, that two other ops, beside a var read, don't emit in a statement context either and therefore have same issue. I created http://dev.clojure.org/jira/browse/CLJS-403 for the REPL change.

Updated attached patch contains just the optimization.

Comment by David Nolen [ 23/Oct/12 6:49 PM ]

fixed, http://github.com/clojure/clojurescript/commit/97e5fbd1e1597d58be35fd8320c8044ccc9d3a3d





[CLJS-394] PersistentTreeSet lookup bug Created: 15/Oct/12  Updated: 27/Jul/13  Resolved: 15/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Erik Ouchterlony Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, patch

Attachments: File bugfix-PersistentTreeSet-lookup.diff    
Patch: Code and Test

 Description   

The lookup function in PersistentTreeSet behaves differently in clojurescript than in clojure when a custom comparison function is used. If two elements are evaluated as equal, one of then is in the set and we do a lookup on the other, clojure returns the element in the set, whereas clojurescript returns the lookup element.

(let [compare-quot-2 #(compare (quot %1 2) (quot %2 2))
s (sorted-set-by compare-quot-2 1)]
(get s 0))

Should return: 1
Returns: 0



 Comments   
Comment by David Nolen [ 15/Oct/12 10:02 PM ]

fixed, http://github.com/clojure/clojurescript/commit/409fcc9a824668207953b2bc47d40aaab85191d3





[CLJS-370] Incorrect behavior of integer? for integral floating point expressions Created: 03/Sep/12  Updated: 27/Jul/13  Resolved: 05/Sep/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-370-v1.patch    
Patch: Code and Test

 Description   

This includes infinity, exponential expressions, etc.

See: http://stackoverflow.com/questions/3885817/how-to-check-if-a-number-is-float-or-integer



 Comments   
Comment by David Nolen [ 03/Sep/12 4:26 PM ]

changing the behavior of integer? outside of a more comprehensive numeric discussion is low priority. The current implementation is already suboptimal in its coercion of numbers to strings.

Comment by Brandon Bloom [ 03/Sep/12 5:31 PM ]

I'm not changing the behavior of integer?, I'm fixing it. Also, regarding the suboptimal nature of string coercion, the following benchmark seems to show this isn't an issue at all:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs
Comment by David Nolen [ 04/Sep/12 10:02 AM ]

Those benchmarks are not revealing anything. Advanced compilation is eliminating that code. Try with :simple or :whitespace.

Comment by Brandon Bloom [ 04/Sep/12 11:13 PM ]

sigh I've once again shown my profiling ineptitude. Here's results running with whitespace optimizations:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 924 msecs
[n 5.5], (old-integer? n), 1000000 runs, 1097 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 1009 msecs
[n ""], (old-integer? n), 1000000 runs, 77 msecs
[n 5], (integer? n), 1000000 runs, 210 msecs
[n 5.5], (integer? n), 1000000 runs, 556 msecs
[n 5.0E300], (integer? n), 1000000 runs, 731 msecs
[n ""], (integer? n), 1000000 runs, 78 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 236 msecs
[n 5.5], (old-integer? n), 1000000 runs, 362 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 2885 msecs
[n ""], (old-integer? n), 1000000 runs, 94 msecs
[n 5], (integer? n), 1000000 runs, 216 msecs
[n 5.5], (integer? n), 1000000 runs, 230 msecs
[n 5.0E300], (integer? n), 1000000 runs, 1360 msecs
[n ""], (integer? n), 1000000 runs, 98 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 471 msecs
[n 5.5], (old-integer? n), 1000000 runs, 467 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 771 msecs
[n ""], (old-integer? n), 1000000 runs, 212 msecs
[n 5], (integer? n), 1000000 runs, 882 msecs
[n 5.5], (integer? n), 1000000 runs, 756 msecs
[n 5.0E300], (integer? n), 1000000 runs, 851 msecs
[n ""], (integer? n), 1000000 runs, 213 msecs

Seems like a win on V8 & SpiderMonkey, but a small loss on JavaScriptCore.

Comment by David Nolen [ 04/Sep/12 11:40 PM ]

The slow down on JSC gives me pause. WebKit is ubiquitous in the mobile space.

I'm also not convinced we need to handle Infinity or NaN (though others may differ on that point). Perhaps we can in some debug mode intelligently detect and error out precisely where these kinds of mistakes may occur.

Comment by Brandon Bloom [ 05/Sep/12 1:19 AM ]

It seems like a bad precedent to favor micro-optimization over correctness when speed is easily obtainable through platform interop.

Infinity, NaN, and floating point exponentiation expressions are not integers. I changed the function's behavior to match JVM Clojure's. We're going to need a reliable integer predicate if we want to implement ratios and other compound numeric types.

If you need need a fast predicate for numbers, the number? predicate only checks type. It's behavior also matches JVM Clojure's: it accepts infinity, NaN, etc. If you need a fast integer test, but insist that edge cases like 5e300 are exceptional and ignorable, then you can micro-optimize by calling the necessary javascript primitives yourself.

Comment by David Nolen [ 05/Sep/12 9:55 AM ]

On my machine your patch is slower across all JS engines w/ JSC faring the worst at nearly 2X. I do agree that the behavior is better. However moving forward this means that even simple functions like even? are 100X slower than their Clojure JVM counterparts. There's nothing "micro-optimization" about 2 orders of magnitude.

Comment by David Nolen [ 05/Sep/12 10:06 AM ]

Oops after a lot more testing and playing around with your patch I turned out to be very wrong! It looks your patch + a boolean type hint is a win across the board on all engines, both in terms of performance and correctness.

Comment by David Nolen [ 05/Sep/12 10:07 AM ]

fixed, http://github.com/clojure/clojurescript/commit/3c210d9430b30ffeac9600ae4851e1c347c2cced

Comment by Brandon Bloom [ 05/Sep/12 11:40 AM ]

Heh. Performance is hard





[CLJS-369] gensyms break re-analyze; prevents variable shadowing analysis Created: 01/Sep/12  Updated: 27/Jul/13  Resolved: 15/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-369-v2.patch     Text File shadowing.patch    
Patch: Code

 Description   

From my email discussion with dnolon before making this patch:

The current analyzer does some trickery to prepare for emitting to JavaScript. Among this trickery is gensyms for locals (including "this"), the new $ prefix on some namespaces, uniqify on parameters, and more. This must be mildly annoying for people writing alternate compiler backends, but for the most part non-blocking because fewer symbol collisions should never be an additional problem for a target language with different symbol resolution rules.

[snip lots more text]

Consider what an ANF transform would look like for the :let form:

(defmethod anf :let
  [{:keys [env bindings statements ret form] :as ast}]
  (let [bindings (mapcat (fn [{:keys [sym init] :as binding}]
                           [name (anf init)])
                         bindings)
        body-env (-> bindings last :env)]
    (ana/analyze env `(~(first form) [~@(map :form bindings)]
                          ~@(anf-block (assoc ast :env body-env))))))

Simple enough, right? This walks each binding, ANF transforms the init expression, gets the environment in which to analyze the body, and then analyzes a new let (or loop) statement with the modified bindings.

Unfortunately, this doesn't work. When the ana/analyze call happens, body-env contains gensymed locals. The result is that body-env is invalidated by the outer analyze call, which is re-generating the symbols for the local variables. If you take the gensyms out of analyze-let, then analyze becomes pure (modulo def forms) and this code magically becomes correct. I've run into this same problem anywhere gensyms are used in analyze.

Commit message on the patch:

AST Changes
    
    * Anywhere a binding was introduced for a local used to be a symbol,
      now it is a map with a :name key and potentially a :shadow key.
    
    * Bindings vectors are no longer alternating symbols, then init maps.
      Instead, the are a vector of maps of the shape described for locals
      plus an :init key.
    
    * The :gthis key for functions has been replaced with :type, which
      is the symbol describing the type name of the enclosing deftype form.
    
    * recur frames now expose :params as binding maps, instead of :names
    
    Benefits:
    
    * Shadowed variables are now visible to downstream AST transforms.
    
    * :tag, :mutable, and other metadata are now uniform across ops
    
    * Eliminates usages of gensym inside the analyzer, which was a source
      of state that made the analyzer impossible to use for some
      transformations of let, letfn, etc which require re-analyzing forms.
    
    * Removes JavaScript shadowing semantics from the analyze phase.


 Comments   
Comment by David Nolen [ 03/Sep/12 1:13 PM ]

Can we please get the ticket number in the first line of the patch? If you look at the ClojureScript commit history you can see the convention that's been adopted. Thanks!

Comment by Brandon Bloom [ 03/Sep/12 2:02 PM ]

Reworded commit message to meet new convention.

Comment by David Nolen [ 28/Sep/12 5:55 PM ]

Can we put the shadow patch in another ticket with the patch referencing the new ticket #. Thanks!

Comment by Brandon Bloom [ 28/Sep/12 6:11 PM ]

Not sure what good a separate ticket would do. How about this new title?

Comment by David Nolen [ 29/Sep/12 12:39 PM ]

They are separate patches. One is a enhancement to the compiler. The other one is an enhancement to my simplistic shadowing solution using the improvements to the compiler in the other enhancement. Thanks!

Comment by Brandon Bloom [ 30/Sep/12 12:34 PM ]

Are you talking about the namespace shadowing? This patch affects all variable shadowing. For example, (fn [x x] x) will produce `function (x x__$1) { return x__$1; }` instead of `function (x_G12 x_G13) { return x__G13; }` or something like that.

I'm not sure how I can break this patch down into smaller pieces. All of the gensyms were there before to eliminate potential shadowing; the two issues are tightly related. If you eliminated all the gensyms, the compiler would work fine... unless you shadowed a variable (which several tests cover).

Have you studied the patch? Can you suggest a concrete way to break it up into smaller patches? I'm not sure it's worth the trouble.

Comment by David Nolen [ 03/Oct/12 10:59 AM ]

Right sorry, makes sense now. I will review both patches more closely. Thanks again.

Comment by David Nolen [ 15/Oct/12 10:43 PM ]

I've finally found some time go through this patch. It's great but it no longer applies to master. If I get an updated patch I';; apply promptly. Thank you very much and sorry for the delay.

Comment by David Nolen [ 15/Oct/12 11:01 PM ]

Went ahead and applied it, only needed a minor change.

Comment by David Nolen [ 15/Oct/12 11:02 PM ]

fixed, http://github.com/clojure/clojurescript/commit/19afb31a52504293ba2182c584b1867917316662

Comment by Brandon Bloom [ 15/Oct/12 11:45 PM ]

Awesome. Glad to see this applied.

Off topic: I like to keep tabs on my open source contributions semi-automatically using the author information stored in git. Sadly, your minor change switched the author to you. If it's not too much to ask, could you please preserve the author info in the future? Either via a separate fixup commit, or by using the --author flag when editing commits. Thanks!

Comment by David Nolen [ 17/Oct/12 10:53 AM ]

Brandon, yep of course! I didn't know about the --author flag otherwise I would have used it. Thanks again.





[CLJS-360] Analyzer incorrectly merges metadata when redefining vars Created: 26/Aug/12  Updated: 27/Jul/13  Resolved: 31/Aug/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File redef-meta-2.patch     Text File redef-meta.patch    
Patch: Code

 Description   

I'm experimenting with using the analyzer for some more sophisticated macros, including a CPS transform and control constructs. During interactive development, I discovered that the analyzer is incorrectly merging metadata on vars when redefining them. This patch changes redef's to replace, rather than merge, existing var metadata.

The patch does not include a test, since the tests don't currently muck with the analyzer directly. Here's some code you can play with in your repl:

(require '[cljs.analyzer :as ana])
(require '[cljs.compiler :as comp])

(def env (ana/empty-env))

(defn show-foo [form]
  (ana/analyze env form)
  (-> @ana/namespaces (get 'cljs.user) :defs (get 'x) :foo))

(show-foo '(def ^{:foo 1} x 1))

(show-foo '(def ^{:foo 2} x 1))

(show-foo '(def x 1))  ; before patch, this returns 2. After patch, nil.


 Comments   
Comment by Brandon Bloom [ 26/Aug/12 6:27 PM ]

Also, the patch makes the behavior match JVM Clojure:

user=> (def ^:foo x)
#'user/x
user=> (:foo (meta #'x))
true
user=> (def x)
#'user/x
user=> (:foo (meta #'x))
nil

Comment by David Nolen [ 31/Aug/12 9:29 AM ]

Can we get a patch without whitespace changes? Thanks!

Comment by Brandon Bloom [ 31/Aug/12 11:22 AM ]

Updated patch: no longer changes indent/whitespace and resolves conflict with change that happened to master since original patch.

Comment by David Nolen [ 31/Aug/12 11:29 AM ]

fixed, http://github.com/clojure/clojurescript/commit/8355d1eacff667b77551bb60699d5c85b2f15298





[CLJS-326] hash-set and PersistentHashSet/fromArray == faster set construction Created: 24/Jun/12  Updated: 27/Jul/13  Resolved: 25/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File making-sets.patch     Text File making-sets-squashed.patch    
Patch: Code and Test

 Description   

As discussed with mmarczyk in irc, the attached patch implements Clojure's hash-set function and modifies the compiler to create sets using a new fromArray static method. To test these, I created three new benchmarks. Here's before and after benchmark results:

$ time ./script/benchmark | grep '#{'
[], #{}, 100000 runs, 77 msecs
[], #{1 2 3}, 100000 runs, 582 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 104 msecs
[], #{}, 100000 runs, 740 msecs
[], #{1 2 3}, 100000 runs, 1809 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 221 msecs
[], #{}, 100000 runs, 281 msecs
[], #{1 2 3}, 100000 runs, 1310 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 288 msecs
./script/benchmark 109.41s user 3.82s system 127% cpu 1:28.96 total

$ time ./script/benchmark | grep '#{'
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 333 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 100 msecs
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 1670 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 325 msecs
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 689 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 321 msecs
./script/benchmark 103.94s user 4.89s system 108% cpu 1:39.88 total
grep '#{' 0.00s user 0.00s system 0% cpu 1:39.85 total



 Comments   
Comment by Brandon Bloom [ 24/Jun/12 8:47 PM ]

Slightly better patch with the EMPTY case

Comment by David Nolen [ 24/Jun/12 10:13 PM ]

Getting a weird error when I try to run the tests. Also can we squash the commits? Thanks!

Comment by Brandon Bloom [ 24/Jun/12 10:24 PM ]

Squashed patch.

Comment by Brandon Bloom [ 24/Jun/12 10:28 PM ]

Squashed and rebased on latest master. Is the rule no-multi-commit patches? Or simply just not for such small patches? In this case, I made and tested each each change in sequence. I also wanted the benchmarks to have a different sha1, so that the perf improvement would show up in the graphs. I'll try to provide squashed patches in the future.

What weird error are you getting? Works for me :-P

Comment by Brandon Bloom [ 24/Jun/12 10:33 PM ]

Updating 2nd benchmark in description to include the EMPTY optimization

Comment by David Nolen [ 25/Jun/12 6:33 AM ]

fixed, http://github.com/clojure/clojurescript/commit/c60df78e2fd318109a9338a87277c11565b506ae





[CLJS-321] Support with-out-str Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 22/Oct/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Paul deGrandis
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: File cljs-321-with-out-str.diff     File cljs-321-with-out-str.diff     Text File with-out-str.patch    
Patch: Code and Test

 Description   

Attached patch implements with-out-str macro in terms of goog.string.StringBuffer and print-fn via .append

The attached patch also fixes CLJS-319



 Comments   
Comment by David Nolen [ 18/Jun/12 8:44 AM ]

Patch looks mostly OK. Did you do any basic benchmarking? It would be nice to know that this doesn't slow down printing of Clojure objects.

Comment by Brandon Bloom [ 18/Jun/12 5:03 PM ]

Yes, I ran this:

(time (dotimes [i 5000] (prn-str {:foo [1 "two" 'three]}))

before patch: 29988 msecs
after patch: 28751 msecs

Effectively identical.

Comment by David Nolen [ 18/Jun/12 8:56 PM ]

on V8? JSC? SM?

Comment by Brandon Bloom [ 19/Jun/12 12:35 AM ]

I had run it in a browser repl, but I forget which. I added a benchmark and re-ran it in all three. It was a tiny bit slower, so I went to investigate and discovered that I haven't the slightest clue how to predict or interpret Javascript engine performance numbers. Hell, Chrome's console seems to have completely random performance, where as the Node.js repl seems to be more consistent. I'll get my shit together and see if I can fix this up. Hopefully will have time to look at it tomorrow.

Comment by Laszlo Török [ 19/Jun/12 3:54 AM ]

jsperf.com was a great help to me when prototyping PersistentVector's first CLJS implementation

it's very-very handy and easy to use and keep updated if necessary

e.g.
http://jsperf.com/persistentvector-norecur-js/12

Comment by David Nolen [ 29/Aug/12 8:42 PM ]

Thanks Brandon, let's hold off on this one until we can resolve CLJS-340

Comment by Paul deGrandis [ 22/Oct/12 4:57 PM ]

I attached an updated patch (that I'm using internally for a personal project) of just the with-out-str macro code.

Comment by David Nolen [ 22/Oct/12 5:00 PM ]

Thanks Paul. Could we get a patch with tests included?

Comment by Paul deGrandis [ 22/Oct/12 5:02 PM ]

No problem - on it right now.

Comment by Paul deGrandis [ 22/Oct/12 5:18 PM ]

Updated with one additional piece I needed to bring over from my branch (:dynamic on print-fn) and two tests: one using print and one using the raw print-fn.

Comment by David Nolen [ 22/Oct/12 6:06 PM ]

fixed, http://github.com/clojure/clojurescript/commit/eab6032e6ba22571e5c4f821707bf5de8075e757





[CLJS-320] memfn not supported Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 18/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File memfn.patch     Text File memfn-with-tests.patch    
Patch: Code and Test

 Description   

The docs say "it almost always preferable to do this directly now", but this was one more thing that slowed down the porting of a small piece of JVM Clojure code for me. I'm of the opinion that parity with Clojure proper is desirable where it doesn't hurt platform interop or performance.



 Comments   
Comment by David Nolen [ 18/Jun/12 8:53 AM ]

Can we get a simple test with this patch?

Comment by Brandon Bloom [ 18/Jun/12 4:55 PM ]

Updated patch with tests

Comment by David Nolen [ 18/Jun/12 9:01 PM ]

fixed, http://github.com/clojure/clojurescript/commit/8965df74d424ed41beb3990a55d775d2d851aacd





[CLJS-319] missing spaces when printing the same thing more than once Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 05/Jul/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tom Jack Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: patch, patch,
Environment:

9ad79e1c


Attachments: Text File cljs-319.patch    
Patch: Code and Test

 Description   

E.g. (println 1 1) prints "11\n" instead of "1 1\n".

Here's the culprit:
https://github.com/clojure/clojurescript/blob/master/src/cljs/cljs/core.cljs#L6136-6137



 Comments   
Comment by Brandon Bloom [ 18/Jun/12 2:14 AM ]

Both pr-with-opts and pr-sb are affected. See:

https://github.com/clojure/clojurescript/blob/master/src/cljs/cljs/core.cljs#L6107

Comment by Brandon Bloom [ 18/Jun/12 3:16 AM ]

Patch here: CLJS-321

Comment by Brandon Bloom [ 21/Jun/12 6:19 PM ]

Attaching a simple patch for this bug instead of fixing it along side with-out-str

Comment by David Nolen [ 05/Jul/12 3:06 PM ]

fixed, http://github.com/clojure/clojurescript/commit/280ea95938883f97be82267d109342178a2e47aa





[CLJS-304] Comparing a js/Date to nil throws Created: 05/Jun/12  Updated: 27/Jul/13  Resolved: 18/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File date-equiv2.patch     Text File date-equiv3.patch     Text File date-equiv.patch    
Patch: Code

 Description   

(= (js/Date.) nil)



 Comments   
Comment by Brandon Bloom [ 05/Jun/12 10:27 PM ]

D'oh! Sorry, this patch causes a warning:

WARNING: Use of undeclared Var cljs.core/instance? at line 384

Comment by David Nolen [ 17/Jun/12 12:06 PM ]

updated patch please.

Comment by Brandon Bloom [ 17/Jun/12 4:28 PM ]

Sorry, forgot about this ticket. Attached patch has two additional commits: 1st stops calling .getTime with list/EMPTY via unecessary '() and the 2nd moves instance? from the "preds" to "fundamentals" section of core.cljs.

Comment by David Nolen [ 18/Jun/12 9:01 AM ]

Thanks can we collapse the patch into one commit?

Comment by Brandon Bloom [ 18/Jun/12 4:45 PM ]

Same as patch 2 with commits squashed

Comment by David Nolen [ 18/Jun/12 8:59 PM ]

fixed, http://github.com/clojure/clojurescript/commit/80726438d7375eb72cd4ea82a7ea676d3237b6ce





[CLJS-301] Inline instance? Created: 05/Jun/12  Updated: 27/Jul/13  Resolved: 15/Jul/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-301-v002.patch     Text File inline-instanceof.patch    
Patch: Code

 Description   

See discussion on http://dev.clojure.org/jira/browse/CLJS-300

This is a low priority patch for inlining instanceof checks. I don't know if it has any significant performance improvement, but the patch was sort of a side effect of my work on CLJS-300, so I figured I'd bundle it up and share it here in case someone wants to test it's perf impact and apply if it is a win.



 Comments   
Comment by David Nolen [ 22/Dec/12 3:29 PM ]

This will probably result in a minor performance boost, but it is nice to get another jsSTAR out of core.cljs. If you update the patch to work on master, I will apply it.

Comment by Brandon Bloom [ 15/Jul/13 4:47 PM ]

Was fixed awhile ago by another patch





[CLJS-300] RegExp objects print incorrectly Created: 04/Jun/12  Updated: 27/Jul/13  Resolved: 04/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File print-regexp2.patch     Text File print-regex.patch    
Patch: Code and Test

 Description   

#"x" is currently printing as #</x/>

js/RegExp needs an IPrintable implementation: (-pr-seq [re _] (list "#\"" (.-source re) "\""))

Unfortunately, this is blocked by the same GClosure issue as http://dev.clojure.org/jira/browse/CLJS-68



 Comments   
Comment by David Nolen [ 04/Jun/12 3:58 PM ]

The simplest solution is to add a RegExp case to pr-seq.

Comment by Brandon Bloom [ 04/Jun/12 6:09 PM ]

That was surprisingly not that simple...

(instance? js/RegExp obj) compiles to `cljs.core.instance_QMARK_.call(null, RegExp, obj)` which also triggers the GClosure warning. So I thought I could inline `instance?` with a macro, but the instanceof operator's operands are in reverse order of instance?'s arguments. To preserve evaluation order, the expansion would be `(let t# ~t ('js* "({} instanceof ~{})" ~o t#))) however even that triggers the warning because the output leaves RegExp as an expression by itself again. I wound up just special-casing simple instanceof checks against symbols in the macro.

Attached patch fixes printing of RegExp objects and also enables calling (instance? js/RegExp X) without triggering the warning. Sadly, any more complex expression which evaluates to js/RegExp, or any higher order usage of instance? against that type will still trigger the warning.

Comment by Brandon Bloom [ 04/Jun/12 7:15 PM ]

As discussed, will make a separate ticket for inlining 'instance? and will (defn regexp? [o] (js* "~{o} instanceof RegExp"))

Comment by Brandon Bloom [ 04/Jun/12 8:46 PM ]

Updated with simplified patch

Comment by David Nolen [ 04/Jun/12 11:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/63cd8ce9c7c9a8864deb676f2b855b8018698d57





[CLJS-297] Eliminate :meta, :vector, :set, and :map ops Created: 03/Jun/12  Updated: 27/Jul/13  Resolved: 15/Jul/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File emit-ds-via-macros.patch    
Patch: Code

 Description   

The attached patch eliminates the :meta, :set, :vector, and :map ops.

These four operations can be defined more simply in terms of
calls to with-meta, set, vector, and hash-map respectively.

The compiler was optimizing construction of vectors and maps. Now,
those optimizations are implemented as macros. Additionally, sets
are optimized in much the same way.

3 files changed, 52 insertions, 99 deletions

Also worth mentioning: as macros instead of ops & emit methods, these optimizations can apply to any backend. The macros create ClojureScript forms, rather than manually generating JavaScript.



 Comments   
Comment by Brandon Bloom [ 17/Jun/12 4:40 PM ]

I'd also like to extend this for Symbols and Keywords.

I've been experimenting with fleshed out Symbol and Keyword objects with interning. I've found that I need emitters, macros, and functions. With the approach here, I could eliminate the emitters and instead have the analyzer produce invocation forms.

Comment by Raphaël AMIARD [ 18/Jun/12 1:20 PM ]

I think this is an interesting patch. It would be worth adapting it to the decoupled emitters. It raises the question of how it would be possible to share part of the emitters between backends.

Comment by David Nolen [ 18/Jun/12 3:25 PM ]

I don't see many benefits falling out of this patch. How to best emit language primitives like literals and constants may vary from host to host - perhaps emitting bytecode directly will work best for some implementations.

Comment by Brandon Bloom [ 18/Jun/12 5:18 PM ]

Couldn't macros emit bytecode via a mechanism similar to the js* form?

My goals with this are:

1) Move some optimizations from the emit phase further down the pipeline. For example, consider choosing the best associative data structure to create. Why should {:foo "bar"} be optimized to an ObjMap or ArrayMap but (hash-map :foo "bar") not be? Why should that optimization be implemented in such a way that it can not be reused by alternative backends?

2) Operate at a higher level. Prefer working with Clojure forms over target-language code fragments (either strings or byte codes). This is where the code length savings is coming from.

If we continue with this approach, I see 4 or 5 more places where analyzer & emitter code can be replaced with shorter, simpler macros, which are more readily reused by alternate backends.

The one implication (downside?) this approach has on consumers of the analyzer or API is that they may need to do a little extra work when considering :invoke operations for static analysis and the like. However, that seems likely for most analyzers anyway, so this would be a matter of (defmethod handle-special-form :map) vs (defmethod handle-invoke :hash-map)

Comment by Michał Marczyk [ 18/Jun/12 5:53 PM ]

Re: 1, I don't think we should be "optimizing" hash-map or array-map (or similar) calls. These functions are a documented way of requesting a map of a particular type (see the docstrings) which I think should not be removed. If anything, we might want to introduce an obj-map function to create arbitrarily large ObjMaps on request (in fact I'll look into that, but that is a separate discussion).

Additionally, the fact that {} is optimized to be a ObjMap in CLJS goes to show that any map-emitting macro will need to be rewritten for each target platform (ObjMap only makes sense when targeting JS, so this optimization simply won't be applicable to other backends). If so and assuming hash-map & Co. retain the behaviour advertised in their docstrings, there's not much gain to implementing this in a macro over just writings a bunch of emitters.

As for decoupling emitters – I think it's perfectly fine for them not to be decoupled, they are the layer closest to the platform after all. Certainly if there's some code which turns out to look the same across multiple platforms it might be worth it to move it upwards in the stack (not necessarily, though – moving it sideways, to a utility namespace / library, might turn out to be more appropriate), but I have a feeling this is an issue best decided once there actually are multiple backends in place and the various costs and benefits can be judged properly.

Now, the story might well be different if we were to introduce some generic factory functions – "create a map of some type", "create a set of some type" etc. – if (and only if!) they would be meant for public consumption. Then implementing a bunch of compiler macros around those new factories and letting them handle data structure literals would save some duplicate work. I don't want to pronounce an opinion on the usefulness of such generic factory functions at this time – just pointing out the possibility.

Comment by Brandon Bloom [ 23/Jun/12 5:14 PM ]

> These functions are a documented way of requesting a map of a particular type

D'oh! You're right.

> we might want to introduce an obj-map

I see you did just that with CLJS-322 – nice.

> the story might well be different if we were to introduce some generic factory functions

There are already some generic factory functions. 'set, for example, is documented as "Returns a set of the distinct elements of coll." despite always returning a PersistentHashSet. Similar for vector and some others. It seems like map is the only core data structure that realistically has several reasonable choices for a default representation.

> implementing a bunch of compiler macros around those new factories and letting them handle data structure literals would save some duplicate work

So all this was somewhat inspired by tagged_literals.clj – You'll see that those functions are effectively macros which take a form and, generally, return an invocation form.

In my mind, I see Clojure's sugar syntax as a strict expansion transformation.

For example, ^:m {:x [@y 'z/w true]} is simply a shortcut for:

(with-meta (make-map (keyword "x") (vector (deref y) (symbol "z" "w") Boolean/TRUE)) (make-map (keyword "m") Boolean/TRUE))

This sort of thing already happens for @ derefs, # lambdas, etc.

In theory, this could be implemented at a level lower than the compiler. You could, for instance, define a reader "desugar" mode which only returns lists and primitives instead of vectors, maps, etc. This would greatly reduce the number of special forms in the compiler, since all of these boil down to invocations with macros.

Emit methods could be replaced with macros for at least these things: vars, maps, vectors, sets, nil, bools, regexes, keywords, symbols, metadata, and empty lists.

The result would be a significant reduction in the amount of code in the compiler for a proportionally smaller increase in the amount of code in per-language macros and maybe the reader.

> I have a feeling this is an issue best decided once there actually are multiple backends in place

I'll grant you that.

I've said my piece on the topic and don't feel very strongly about this particular patch. I just wanted to spark the discussion about reusing more bits of the compiler between backends. In my mind, it's almost always preferable to transform lists than it is to emit strings. I tried that, and the result was a reduction in responsibilities for the analyzer and macros that were easier to work with than emit methods.

Comment by Michał Marczyk [ 24/Jun/12 7:41 PM ]

Some further discussion here:

http://clojure-log.n01se.net/date/2012-06-24.html#20:30a

Comment by Brandon Bloom [ 16/Aug/12 9:34 PM ]

One other advantage of function application over special casing maps/sets/etc is that argument evaluation order is well defined for function application (left-to-right). The Clojure reader returns un-ordered maps & sets, so without changing the reader, we have no way of being able to know what order map key-value-pairs or set elements were originally in. I filed a bug on that. I think we need to make the reader extensible to say to create the return values from their children expressions. In the case of the ClojureScript compiler, we do care about order, so we'd want to return either a (make-map ...) form directly, or a sorted-map by read-order. Same goes for sets.

Comment by David Nolen [ 31/Aug/12 9:24 AM ]

There's not enough rationale for this one.

Comment by Brandon Bloom [ 15/Jul/13 4:50 PM ]

David and I resolved this in a different (better) way:

https://github.com/clojure/clojurescript/commit/f80956d90f455810be140cfec1632f55254385a5





[CLJS-294] AST contains munged symbols Created: 02/Jun/12  Updated: 27/Jul/13  Resolved: 03/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File emit-munge.patch    
Patch: Code

 Description   

All of the symbols in the abstract syntax tree are munged according for use in JavaScript. This is a problem because it means the AST is lossy and new compiler backends may have differing munging rules.

The attached patch removes all munging from the analysis phase and moves it into the emit phase. The emit phase is backend specific, so it's an appropriate place for munging.



 Comments   
Comment by David Nolen [ 03/Jun/12 9:55 AM ]

It may be better to formalize munging as there are some other bits different backend must do, Raphael Amiard has taken some notes on this here: http://github.com/raph-amiard/clojurescript/wiki/How-to-modularize-parse-analyze-phases-in-the-compiler

Comment by Brandon Bloom [ 03/Jun/12 11:33 AM ]

I agree that we'll eventually want to formalize munging. However, whenever that happens, it should still happen after the AST is constructed. Munging shouldn't happen at all if you're doing refactoring or the like, where you query the AST. Munging is code generation concern.

Prior to my patch, the AST needs to support both :name and :name-sym keys to get around the fact that the :name key loses fidelity of the original symbol. This complects the Clojure-specific parse phase with target-specific needs. The patch renames :name-sym to replace :name.

I read Raphael's notes and he and I have a email thread going on too. With that in mind, this is a step towards untangling the architectural layers of the compiler. By better segregating parse from emit, it will be easier to break the analyzer out from the code generation backend.

Comment by David Nolen [ 03/Jun/12 5:12 PM ]

fixed, http://github.com/clojure/clojurescript/commit/a9d1dc6a043e8a6367492a2cf8622f73f2a947f9





[CLJS-289] Represent ast :children as a vector of keys Created: 31/May/12  Updated: 27/Jul/13  Resolved: 05/Jun/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: patch, patch,

Attachments: Text File children-keys.patch    
Patch: Code

 Description   

See discussion here: https://groups.google.com/d/topic/clojure-dev/vZLVKmKX0oc/discussion

Summary: Duplication of ast nodes in various keys and in :children is problematic for some users. In particular, it doesn't play nicely with printing. A solution is needed which preserves "The AST is data. Period."

The attached patch makes no changes to how the sub nodes are currently stored outside of the :children key. Instead, it replaces :children values with a vector of keys into the main ast node. This preserves child ordering and allows a children function to be defined as:

(defn children [ast]
(mapcat #(if (coll? %) % [%]) ((apply juxt (:children ast)) ast)))

The attached patch has two caveats: 1) Many (but not all?) blocks will now be present in the children hierarchy as 'do forms. 2) Interleaved forms are intentionally bugged with respect to ordering. The :children vector for those forms match the actual behavior, not the expected behavior. This can be fixed along with http://dev.clojure.org/jira/browse/CLJS-288



 Comments   
Comment by David Nolen [ 03/Jun/12 10:00 AM ]

the result of the discussion did not end with agreement on representing :children as a vector of keys

Comment by Brandon Bloom [ 03/Jun/12 11:24 AM ]

I realize that, but it was one approach suggested and seemed entirely viable, so I tried it. From what I can tell, it seems like a solution that meets all of the criteria that came up during the discussion.

Did I overlook a requirement?

Who is currently using :children and could weigh in on whether or not this still meets their needs?

Comment by Jonas Enlund [ 03/Jun/12 11:52 AM ]

I'm using :children and I don't think this patch (currently) meets my needs. At least {:op :let ...} is broken IMO. (something like `(map :init bes)` is missing from the patch)

Comment by Brandon Bloom [ 03/Jun/12 12:09 PM ]

Ah OK, you're right. It's the same problem there that I ran into with :statements and analyze-block: There is a pseudo AST node that needs to be traversed down into.

The easy fix for that here would be to merge {:children [:init]} into each binding, but the binding hash will be missing :op and :form. For analyze-block, I was able to use :op :do and :form (list 'do ...), but there isn't an obvious analogy here without inventing a :binding op or relaxing the requirement for :op and :form.

Interestingly, each successive binding can be treated as a single nested binding. What if we defined let as a macro which expands to a series of let1 forms? (let [x 1 y 2] (* x y)) --> (let1 [x 1] (let1 [y 2] (* x y))

That may increase the depth of the AST, but it would really simplify traversal into binding clauses. Each let op would have :name, :init, and children as [:init].

In general, it may be worth while to consider doing the same thing with the various analyze-blocks situations, such that 'do is the only place multiple statements can occur.

Comment by Brandon Bloom [ 03/Jun/12 12:16 PM ]

Er, I mean: a hypothetical let1 op would have children [:init :statements :ret]

However, if you also expanded the implicit do blocks, you'd be able to simplify to [:init :body] or something like that.

Comment by Brandon Bloom [ 03/Jun/12 12:34 PM ]

Forgive me for repeatedly commenting, but it also occurs to me that you can change 'do to a macro which expands to a series of 'do2 forms:

(do x y z) --> (do2 x (do2 y z)) or (do2 (do2 x y) z)

That do2 op could have children [:left :right] and all other :statements and :ret children could be simplified down to 'do2 expansions.

With that in mind, the children fn becomes simply (defn children [ast] ((apply juxt (:children ast)) ast)) and a lot of analyze and emit code gets much simpler by not having to map over many collections all over the place.

Kinda a wild idea and probably not the right forum for it, but worth suggesting. Thoughts?

Comment by David Nolen [ 04/Jun/12 11:35 AM ]

I see little benefit to changing core macros. I also see no problem with creating a :binding ast node.

Comment by Jonas Enlund [ 05/Jun/12 12:44 AM ]

Every node (i.e. an map with :op, :form and :env keys, called an "Expression Object" in the docstring for analyze) in ClojureScript corresponds to a self-contained expression. A binding form is not self-contained, it is a part of the let expression.

Another similar example is functions. There is a node {:op :fn ...} but there is no {:op :fn-method} even thou there are maps under the :methods key describing function methods. The same argument goes for this: A function method is not self-contained, it is part of the function and should not be a node.

The nodes (expression objects) which make up the ClojureScript AST seems to be really well thought out and I would be careful in making the proposed change.

Comment by David Nolen [ 05/Jun/12 8:09 AM ]

Agreed I think it's far too early to have a ticket for this. Confluence page first.

Comment by Brandon Bloom [ 29/Aug/12 12:09 AM ]

Design page here: http://dev.clojure.org/display/design/AST+children





[CLJ-1519] Added extra arity to clojure.core/ns-* fns Created: 04/Sep/14  Updated: 10/Sep/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Baranosky Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: enhancement, patch

Attachments: Text File new-ns-arity.patch    
Patch: Code and Test

 Description   

Hello,

Adds another arity where the "ns" parameter is set to a default value of *ns* in these fns:

ns-unmap, ns-resolve, ns-name, ns-map, ns-publics, ns-imports, ns-interns, ns-refers, ns-aliases, ns-unalias

I find I very often use ns-unalias and ns-unmap from the repl, and passing the *ns* arg gets a little tedious.






[CLJ-1514] Use qualified class names for return type hints of standard Clojure functions Created: 28/Aug/14  Updated: 28/Aug/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, interop, patch, typehints

Attachments: Text File 0001-Use-fully-qualified-class-names-for-return-type-hint.patch    
Patch: Code

 Description   

The attached patch converts all function return type hints to spell out the class name fully qualified. There are two reasons for doing this:

1. Simple names in return type hints cause the issue described in http://dev.clojure.org/jira/browse/CLJ-1232. That's usually not a problem with return type hints referring to java.lang-classes because those are always imported. However, using `ns-unmap` you can remove them. For example, after `(ns-unmap ns 'String)` in my namespace, `(.length (format "foo = %s") 1)` throws an IllegalArgumentException: Unable to resolve classname: String. By using fully-qualified class names, that problem goes away.

2. tools.analyzer (used by the Clojure lint tool Eastwood) crashes when encountering such a simple-named return type hint. So currently, I cannot lint parts of my project because there's code that calls `clojure.core/format`.



 Comments   
Comment by Alex Miller [ 28/Aug/14 9:34 AM ]

1. that seems like a pretty weird thing to do
2. sounds like an issue with tools.analyzer, not with Clojure?

Comment by Nicola Mometto [ 28/Aug/14 10:46 AM ]

Just to clarify, tools.analyzer(.jvm) can analyze just fine forms in the form (defn x ^Class []) as long as Class is resolvable, whereas it will throw an exception if that function is then used in a namespace where that class is no longer resolvable, which is similar to what Clojure already does, except tools.analyzer.jvm will throw an exception even if the type hint is not used.

Since version 0.5.1 there's an handler that can be provided to change that behaviour, see https://github.com/clojure/tools.analyzer.jvm/blob/master/src/main/clojure/clojure/tools/analyzer/passes/jvm/validate.clj#L232

Comment by Nicola Mometto [ 28/Aug/14 11:02 AM ]

Now a comment regarding this ticket: the patch in this ticket is just a work-around for the issue exposed in http://dev.clojure.org/jira/browse/CLJ-1232, IMHO the correct move would be to actually recognize that issue as a bug rather than as an accepted "limitation" as Rich's comment seems to suggest so that a fix might be commited.

Comment by Tassilo Horn [ 28/Aug/14 1:29 PM ]

@Alex: 1. is not as weird as it sounds at first. For example, consider you have macros that generate complete APIs for something into some new namespace. Then it can make sense to use a real vanilla namespace, i.e., without referring clojure.core and importing java.lang. With 2. I side with Nicola and consider CLJ-1232 a bug.

@Nicola: Today I've used Eastwood (0.1.4) to lint my project. It crashed when it encountered this definition:

(defmacro error
  "Throws an exception with the given message and cause."
  ([msg]
     `(error ~msg nil))
  ([msg cause]
     `(throw (java.lang.Exception. ~msg ~cause))))

(defmacro errorf
  "Throws an exception with the given `msg` and `objs` passed to `format`.
  `msg` is a format string."
  [msg & objs]
  `(error (format ~msg ~@objs)))  ;; This is line 112 where the crash occurs

The message was:

Exception thrown during phase :analyze+eval of linting namespace funnyqt.tg-test
A function, macro, protocol method, var, etc. named clojure.core/format has been used here:
{:file "funnyqt/utils.clj",
 :end-column 19,
 :column 12,
 :line 112,
 :end-line 112}
Wherever it is defined, or where it is called, it has a type of String
This appears to be a Java class name with no package path.
Library tools.analyzer, on which Eastwood relies, cannot analyze such files.
If this definition is easy for you to change, we recommend you prepend it with
a full package path name, e.g. java.net.URI
Otherwise import the class by adding a line like this to your ns statement:
    (:import (java.net URI))

An exception was thrown while analyzing namespace funnyqt.tg-test 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.

So it seems it crashes because `format` has a `^String` return type hint. The namespace containing the `errorf` macro above has no modified ns-imports, i.e., all java.lang classes are imported there, too.

Comment by Nicola Mometto [ 28/Aug/14 1:46 PM ]

Tassilo, since `errorf` is a macro, that error is probably caused at the expansion point of that macro in a namespace that unmaps 'String.
If that's not the case, please open a ticket in the eastwood repo

Comment by Tassilo Horn [ 28/Aug/14 2:16 PM ]

Nicola, you are correct. As I've explained above to Alex, I generate APIs in fresh namespaces that don't refer clojure.core and also ns-unmap all java.lang classes, and the generated code also contains `errorf`-forms.

Well, since `ns-unmap` is there, I think it's legit to use it. So that makes CLJ-1232 even more important. But until that gets fixed which requires a common agreement that it is indeed a bug, I'd be very happy if this patch could be accepted. I mean, when it cannot do any harm and doesn't obscure anything but helps at least one person, then why not do it?





[CLJ-1203] Fallback to hash-based comparison for non-Comparables in Util.compare() Created: 17/Apr/13  Updated: 29/Apr/13  Resolved: 29/Apr/13

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-Add-hasheq-based-fallback-to-Util.compare.patch    
Patch: Code

 Description   

I oftentimes use sorted collections, and my comparator functions usually look like that:

(fn [a b]
  (let [x (compare-according-to-my-use-case a b)]
    (if (zero? x)
       (compare a b)
       x)))

That is, I define the sorting order depending on the requirements of my use-case, and if that doesn't suffice to make a distinction, I simply fall back to the standard `compare` function in order to get a stable ordering anyhow. I think that's a widely used idiom, e.g., also used in "Clojure Programming" (for example page 109).

The problem with this approach is that several data structures don't implement Comparable, and thus aren't applicable to `compare` (you'll get a ClassCastException). Examples are maps, records, deftypes, and sequences.

The patch I'm going to attach extends `Util.compare()` with a fallback clause that performs a `hasheq()`-based comparison. This results in a meaningless but at least stable sorting order which suffices for use-cases like the one shown above.



 Comments   
Comment by Andy Fingerhut [ 18/Apr/13 9:01 PM ]

Patch 0001-Add-hasheq-based-fallback-to-Util.compare.patch dated Apr 17 2013 applies cleanly to latest master, but causes several unit tests in data_structures.clj to fail. These are the kinds of things you would expect to fail with the change made in the patch, because the failing tests expect an exception to be thrown when comparing objects that don't implement Comparable, and with this patch's changes they no longer do. If this patch's change is desired, those tests should probably be removed.

Comment by Tassilo Horn [ 19/Apr/13 2:34 AM ]

Thanks Andy, I can't believe I've forgotten to re-run the tests.

Anyway, I'm attaching a new version of the patch that deletes the few sorted-set and sorted-set-by invocations that check that an exception is thrown when creating sorted sets containing non-Comparables.

Comment by Tassilo Horn [ 19/Apr/13 2:35 AM ]

New version of the patch.

Comment by Andy Fingerhut [ 26/Apr/13 2:47 PM ]

Tassilo, you say that one of your use cases is sorted collections. Note that any compare function that returns 0 for two values will cause sorted sets and maps to treat the two compared values as equal, and at most one of them will get into the ordered set/map, the second treated as a duplicate, even though the values are not equal. See https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md#comparators-for-sorted-sets-and-maps-are-easy-to-get-wrong for an example (not based on your modified compare, but on a comparator that returns 0 for non-equal items).

With your proposed change to compare, this occurrence would become very dependent upon the hash function used. Probably quite rare, but it would crop up from time to time, and could potentially be very difficult to detect and track down the source.

Comment by Tassilo Horn [ 29/Apr/13 2:29 AM ]

Hi Andy, you are right. It's possible to add an explicit equals()-check in cases the hashcodes are the same, but I think there's nothing you can do in the hashcodes-equal-but-objects-are-not case. That is, there's no way to ensure the rule sgn(compare(x, y)) == -sgn(compare(y, x)), the transitivity rule, and the compare(x, y)==0 ==> sgn(compare(x, z))==sgn(compare(y, z)) for all z rule.

I'm closing that ticket.





[CLJ-1066] No dependency on jsr166y Created: 11/Sep/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Major
Reporter: Wolodja Wentland Assignee: Stuart Halloway
Resolution: Completed Votes: 4
Labels: patch
Environment:

$ java -version
java version "1.7.0_03"
OpenJDK Runtime Environment (IcedTea7 2.1.2) (7u3-2.1.2-2)
OpenJDK 64-Bit Server VM (build 22.0-b10, mixed mode)
$ lein version
Leiningen 2.0.0-preview10 on Java 1.7.0_03 OpenJDK 64-Bit Server VM


Attachments: Text File 0001-Don-t-AOT-compile-clojure.core.reducers.patch    
Patch: Code
Approval: Ok

 Description   

The Clojure 1.5.0-alpha4 jars that have been deployed to maven.org seem to have been compiled against JDK6 which causes
an exception if one tries to use reducers/fold.

— snip —
Setup:

user=> (require '[clojure.core.reducers :as r])
nil
user=> (def v (vec (range 10000)))
#'user/v

;; JDK7 + clojure 1.5.0-alpha4 from maven.org
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]] in project.clj

user=> (r/fold + (r/map inc v))
ClassNotFoundException jsr166y.ForkJoinTask java.net.URLClassLoader$1.run (URLClassLoader.java:366)

;; JDK7 + clojure 1.5.0-alpha4 from maven.org
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]
;; [org.codehaus.jsr166-mirror/jsr166y "1.7.0"]]

user=> (r/fold + (r/map inc v))
50005000

;; JDK7 + clojure 1.5.0-alpha4 locally compiled (mvn install) against OpenJDK7
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]] in project.clj

user=> (r/fold + (r/map inc v))
5000050000
— snip —

It would be wonderful if this issue could be fixed before the release of 1.5.0.

Have a nice day

Wolodja



 Comments   
Comment by Tassilo Horn [ 12/Sep/12 9:44 AM ]

That's really a nasty problem. I wrote the code that makes it possible to compile clojure with either a JDK7 and native ForkJoin or an older JDK + jsr166y. Unfortunately, if you build clojure with a JDK7, reducers' fold requires a JDK7 at runtime, and if you build clojure with a JDK <7 + jsr166y, fold also requires jsr166y at runtime.

The essential problem is that clojure is AOT-compiled to class-files and those are put into the release jars. Ordinary clojure libraries are distributed as jar files containing clj files that are compiled when loaded. There, the implemented approach works just fine. For example, again your example with 1.5.0-alpha4:

;; 1.5.0-master4 compiled with JDK6 + jsr166y running on a JDK7
user=> (require '[clojure.core.reducers :as r])
nil
user=> (def v (vec (range 10000)))
#'user/v
user=> (r/fold + (r/map inc v))
ClassNotFoundException jsr166y.ForkJoinTask  java.net.URLClassLoader$1.run (URLClassLoader.java:366)
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Now load the reducers.clj source code again, so that it
;; picks the right ForkJoin-impl for the current platform
(load-file "/home/horn/Repos/clj/clojure/src/clj/clojure/core/reducers.clj")
nil
user=> (r/fold + (r/map inc v))
50005000

So IMO, the best thing we can do is to omit AOT-compilation for reducers but to include only its clj file so that it'll be compiled automatically on the platform it eventually runs.

Comment by Tassilo Horn [ 12/Sep/12 11:55 AM ]

This patch removes the clojure.core.reducers namespace from the namespaces to be AOT compiled. So now this namespace will be JIT-compiled when being required, and at that point either the JDK7 ForkJoin classes or the jsr166y classes need to be there.

Comment by Stuart Halloway [ 21/Sep/12 1:31 PM ]

Rich: what is the right approach here?





[CLJ-1061] when-first double evaluation Created: 04/Sep/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: patch
Environment:

Mac OS X 10.8.1, Java 1.7.0_06


Attachments: Text File 0001-avoid-double-evaluation-in-when-first.patch    
Patch: Code
Approval: Ok
Waiting On: Rich Hickey

 Description   

The when-first macro will evaluate the xs expression twice. Admittedly, it does exactly what the doc string says, but that seems undesirable to me. Even without side effects, there's a potential performance issue if xs is some expensive operation.

Patch attached. The main diff is:

- `(when (seq ~xs)
- (let [~x (first ~xs)]
- ~@body))))
+ `(when-let xs# (seq ~xs)
+ (let ~x (first xs#)
+ ~@body))))



 Comments   
Comment by Stuart Sierra [ 21/Sep/12 7:39 AM ]

Screened.





[CLJ-1050] Remove a lock in the common case of deref of delay Created: 26/Aug/12  Updated: 18/Sep/12  Resolved: 18/Sep/12

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

Type: Enhancement Priority: Trivial
Reporter: Nicolas Oury Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch, performance

Attachments: Text File 0001-No-lock-in-the-common-path-for-delays.patch    

 Description   

Currently, when deref is called in Delay.java, a lock on the Delay is always acquired.
This is wasteful as most of the time you just want to read the val.

The attached patch changes this behaviour to the following:

  • val is initialised to a known secret value. (During its constructor so it is visible from any thread).
  • When deref is called, val is checked to the known secret value.
  • If it is not the secret value, then it has to be the value computed by the function and we return it.
  • If it is the secret value, then we lock this object and revert to the current behaviour.

This is faster than what is done currently and can be very much faster if there is contention on the delay.



 Comments   
Comment by Nicolas Oury [ 27/Aug/12 2:37 AM ]

Please close and reject. The patch is not working if val has non final fields.





[CLJ-1042] [PATCH] Allow negative substring indices for (subs) Created: 14/Aug/12  Updated: 19/Sep/12  Resolved: 17/Sep/12

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

Type: Enhancement Priority: Minor
Reporter: Ian Eure Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File clj-1042-negative-indices-patch3.txt     Text File negative-subs.patch    
Patch: Code and Test

 Description   

This adds Python-style negative string indices for (subs), e.g.:

(subs "foo bar" -3) ;; -> "bar"



 Comments   
Comment by Andy Fingerhut [ 16/Aug/12 7:17 PM ]

Ian, thanks for the patch. It is Rich Hickey's policy only to consider applying patches to Clojure from those who have signed a Clojure contributor agreement: http://clojure.org/contributing

Were you interested in doing so, or perhaps it is already in progress?

Comment by Ian Eure [ 20/Aug/12 11:44 AM ]

I wasn't aware that this was necessary. I'm mailing the form in.

Comment by Andy Fingerhut [ 27/Aug/12 7:56 PM ]

Patch clj-1042-negative-subs-patch2.txt dated Aug 27 2012 is identical to Ian Eure's negative-subs.patch, except it is in the desired git format.

Ian, for future reference on creating patches in the desired format, see the instructions under the heading "Development" on this page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Ian Eure [ 28/Aug/12 11:47 AM ]

Thanks, will do.

Comment by Steve Miner [ 04/Sep/12 3:53 PM ]

If Clojure decides to support Python-style negative indices, you should also consider adding support to subvec.

Comment by Ian Eure [ 06/Sep/12 12:17 PM ]

Patch extended to support negative indices on (subvec) as well.

Comment by Adrian Bendel [ 07/Sep/12 8:01 AM ]

The arg to rindex should probably be tagged with ^clojure.lang.Counted instead of ^String now.

Comment by Steve Miner [ 07/Sep/12 1:31 PM ]

Regarding the previous comment, String is a Java class so it isn't a clojure.lang.Counted. Is the type hint necessary? Maybe it should be on the call rather than the defn.

Ignoring the type hinting, I'll suggest a slightly simpler way to implement the rindex logic:

(defn rindex [coll i]
(if (neg? i) (+ (count coll) i) i))

In any case, I'm not sure rindex should be public even if you want the subs and subvec enhancements. Someone needs to make the case for adding a new function to core.

The Pythonic negative index is a debatable feature since it's pretty easy to implement for yourself if you want it.

Comment by Adrian Bendel [ 07/Sep/12 11:05 PM ]

Sorry, the type hint on rindex args isn't necessary at all. Just looked up in the source, calling count should never be reflective, since (count ..) emits calls to clojure.lang.RT/count.

Your solution looks good.

Comment by Stuart Halloway [ 17/Sep/12 7:07 AM ]

Negative indices were considered and rejected a long time ago. (I am merely conveying information--I have no strong opinion on this one.)

Comment by Andy Fingerhut [ 17/Sep/12 12:07 PM ]

Note: If some people really like negative index behavior as in Perl or Python, it is straightforward to create a library of functions in a different namespace, perhaps with different names, that can do it. Perhaps a "pythonisms" library?

Comment by Ian Eure [ 18/Sep/12 12:31 PM ]

Would this be accepted as part of clojure.string instead? I considered putting it there, but thought it would be confusing to have multiple substring functions in different namespaces.

This is very helpful in practice, and I'd really like to see at least the (subs) stuff in Clojure.

Comment by Andy Fingerhut [ 18/Sep/12 2:52 PM ]

Disclaimer: I'm no Clojure/core member, so can't speak authoritatively on whether something would or would not be accepted into clojure.string.

However, given that clojure.string is distributed with clojure.core, my guess is it would not be accepted. You'd be able to get things like this out for you and others as a separate library distributed on Clojars. That would also make it easy to include other Python-like things that you don't find in Clojure already.

Comment by Ian Eure [ 18/Sep/12 4:02 PM ]

This isn't about "python-like things," this is about a useful feature. Lots of languages support this: Perl, PHP, Ruby, Python, JavaScript, to name a few. Are you really suggesting that I should create a whole package for a version of a function in clojure.core with slightly different semantics? That's insane.

Anyway, I'm done wasting my time trying to navigate this hopelessly broken process. Maybe it would have been accepted if I included yet another way to interoperate with Java types.

Comment by Michael Klishin [ 18/Sep/12 5:09 PM ]

Stuart, do you remember why specifically negative indexes were rejected? Developing a separate library for a minor improvement to an existing function sounds unreasonable.

Comment by Carlos Cunha [ 18/Sep/12 5:10 PM ]

some explanation about this topic was given by Rich Hickey himself here: http://news.ycombinator.com/item?id=2053908

citation:
"...Yes, there is a backlog from when it was just me, and it will take a while to whittle down. We have finite resources and have to prioritize. I can assure you we have more important things to concentrate on than your negative index substring enhancement, and are doing so. You'll just have to be patient. Or, if you insist, I'll just reject it now because a) IMO it's goofy, b) you can make your own function that works that way c) we don't get a free ride from j.l.String, d) it begs the question of negative indices elsewhere..."

i've been following this thread hoping this feature would be included. but whatever the reason was for the rejection, i'm sure it was thoughtful. great thanks for this wonderful language, and thanks Ian Eure for his effort.

Comment by Steve Miner [ 18/Sep/12 5:25 PM ]

That HN link eventually leads back to CLJ-688 which was rejected.

Comment by Stuart Halloway [ 19/Sep/12 12:03 PM ]

Michael: A proposal for negative indexes would need to be systematic in considering implications for all functions that have index arguments.

Ian: Clojure is driven by design, not incremental piling of features.

All: clojure.string is incomplete in more important and fundamental ways than negative indexes. This sucks now, and will suck worse as more code is written in different dialects. I find myself wishing string was a contrib, so we could iterate faster.

Comment by Andy Fingerhut [ 19/Sep/12 1:34 PM ]

Stuart: Any specific proposals for how you'd like to see clojure.string improve? If it can be made a contrib, that would be cool, but understood if that would be considered too breaking of a change. Even if it isn't made a contrib, having tickets for improvement ideas you are willing to see means patches might get written, and they'll get in some time.





[CLJ-1024] Varargs protococol impls can be defined but not called Created: 10/Jul/12  Updated: 14/Feb/13  Resolved: 13/Feb/13

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

Type: Enhancement Priority: Minor
Reporter: Víctor M. Valenzuela Assignee: Stuart Halloway
Resolution: Declined Votes: 1
Labels: patch

Attachments: Text File 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch    
Patch: Code

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable?



 Comments   
Comment by Tassilo Horn [ 11/Jul/12 1:20 AM ]

First of all, clojure.lang.IFn is no protocol but an interface. And it does not declare a

  public Object invoke(Object... obs)

method. It has an `invoke` method with 20 Object parameters followed by an Object... parameter, but to give an implementation for that, you have to specify every parameter separately, and the last Object... arg is just a normal argument that must be an Object[]. That's because Java-varags Type... parameters are just Java syntax sugar, but in the byte-code its simply a Type-array.

What your example does is provide an `invoke` implementation for the 2-args version, where the first parameter happens to be named `&`, which has no special meaning here. Take that example:

(deftype MyFoo []
  clojure.lang.IFn
  (invoke [this & xs]
    [& xs]))

((MyFoo.) 1 2)
=> [1 2]

But you are right in that `deftype`, `defrecord`, `defprotocol`, and `definferface` probably should error if user's seem to try to use varargs or destructuring.

Comment by Víctor M. Valenzuela [ 11/Jul/12 1:55 AM ]

Cheers for a most clarifying response.

The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion.

Just for the record, destructuring seems to work, at least for interface impls.

Comment by Tassilo Horn [ 11/Jul/12 2:42 AM ]

> The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion.

I agree. I'll attach a patch which checks for those invalid usages soon.

> Just for the record, destructuring seems to work, at least for interface impls.

Could you please provide a complete example demonstrating your statement?

I'm rather sure that varargs and destructuring don't work for any of defprotocol, definterface, deftype, defrecord, and reify. But you can use varargs and destructuring when providing dynamic implementations via `extend` (or `extend-protocol`, `extend-type`), because those impls are real functions in contrast to methods.

Comment by Tassilo Horn [ 11/Jul/12 2:43 AM ]

I attached a patch. Here's the commit message:

Check for invalid varags/destrucuring uses.

Protocol, interface method declarations and implementations don't allow for
varags and destructuring support. Currently, for example

(defprotocol FooBar
(foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that happens to be
named & without special meaning. But clearly, the user wanted to specify a
varags parameter here. The same applies to definterface.

Similarly, providing method implementations via defrecord, deftype, and reify
don't allow for destructuring and varags (but dynamic extenions via extend do).

So this patch makes defprotocol, definterface, defrecord, deftype, and reify
throw an IllegalArgumentException if any argument vector contains a
destructuring form or varargs argument.

Comment by Víctor M. Valenzuela [ 12/Jul/12 3:13 AM ]

Glad you've considered my request.

As for destructuring, I was speaking after this example, which may or may not work like it looks like - I don't know.

(deftype foo []
clojure.lang.IFn
(invoke [this [a b] c] (println a b c)))

((foo.) [1 2] 3)

Comment by Tassilo Horn [ 12/Jul/12 8:22 AM ]

Indeed, descructuring seems to work for method implementations. I'll adapt my patch...

Comment by Tassilo Horn [ 12/Jul/12 8:42 AM ]

Revamped patch. Here's the commit message:

Protocol, interface method declarations don't allow for varags and
destructuring support. Currently, for example

  (defprotocol FooBar
    (foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that happens to be
named & without special meaning. But clearly, the user wanted to specify a
varags parameter here. The same applies to definterface.

Similarly, providing method implementations via defrecord, deftype, and reify
don't allow for varags (but dynamic extenions via extend do).

So this patch makes defprotocol and definterface throw an
IllegalArgumentException if a user tries to use varargs and destructuring in
method signatures.

Similarly, defrecord, deftype, and reify throw an IllegalArgumentException if
any method implementation arglist contains a varargs argument.

Comment by Andy Fingerhut [ 12/Jul/12 3:43 PM ]

Tassilo, with your patch 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch dated July 12, 2012, I get the following error message while testing, apparently because some metadata is missing on the new functions your patch adds to core:

[java] Testing clojure.test-clojure.metadata
[java]
[java] FAIL in (public-vars-with-docstrings-have-added) (metadata.clj:45)
[java] expected: (= [] (remove (comp :added meta) public-vars-with-docstrin
gs-not-generated))
[java] actual: (not (= [] (#'clojure.core/throw-on-varargs-and-destr #'cl
ojure.core/throw-on-varargs)))

Comment by Tassilo Horn [ 13/Jul/12 2:10 AM ]

Hi Andy, this updated patch declares the two new checking fns private which makes the tests pass again.

Stupid mistake by me: Of course, I've tested the last version, too, but afterwards I decided it wouldn't be bad to add some docstrings, and of course, adding docstrings cannot break anything.

Comment by Aaron Bedra [ 14/Aug/12 7:58 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed as a CA signer.

Comment by Aaron Bedra [ 14/Aug/12 8:02 PM ]

This looks ok to me, but it seems like a fair amount of duplication to accomplish the task. It seems like we should just be able to ask if it is ok to proceed, instead of having to pick which function needs to be called in what case.

Comment by Tassilo Horn [ 15/Aug/12 1:23 AM ]

Aaron, can you please elaborate? I don't get what you mean with duplication and asking if it is ok to proceed.

Comment by Tassilo Horn [ 31/Aug/12 1:40 AM ]

Rebased to apply cleanly on master again.

Comment by Víctor M. Valenzuela [ 31/Aug/12 7:11 AM ]

Pardon the silly contribution, but the added methods' usage of double-negations (when-not ...) seems unnecessary.

Comment by Tassilo Horn [ 31/Aug/12 8:03 AM ]

Hi Victor, this revamped patch removes the double-negation and is more concise and clearer as a result. So your comment wasn't as silly as you thought.

Comment by Tassilo Horn [ 14/Feb/13 1:36 AM ]

Hey Stu, do you mind to explain why you've declined the patch?

Comment by Marek Srank [ 14/Feb/13 8:52 AM ]

@Tassilo: https://groups.google.com/forum/#!topic/clojure-dev/qjkW-cv8nog

Comment by Tassilo Horn [ 14/Feb/13 10:03 AM ]

@Marek, Stu: Thanks, I've left a reply there: https://groups.google.com/d/msg/clojure-dev/qjkW-cv8nog/rMNFqbjNj-EJ





[CLJ-1018] range's behavior is inconsistent Created: 29/Jun/12  Updated: 24/May/13  Resolved: 24/May/13

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

Type: Defect Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, range

Attachments: File inconsistent_range_fix.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem statement: The current behavior of range is inconsistent. (range 0 9 0) has always produced (). (range 0 9 -1) has always produced (). (range 9 0 1) has always produced (). However, (range 9 0 0) produces (9 9 9 9 ...), and (range 0 0 0) produces '(0 0 0 0 ...)

Proposal: Make the behavior of range consistent when using a step of 0 to make it produce an empty list.

Please see attached code and patch.



 Comments   
Comment by Mike Anderson [ 01/Jul/12 4:08 AM ]

Agree it is good to fix the inconsistency, but I think an infinite sequence of zeros is the correct result, as implied by the current docstring definition.

It's also mathematically cleanest: range should produce an arithmetic progression until the end value is equalled or exceeded.

Empty lists only seem to make sense as a return value when start >= end.

Comment by Devin Walters [ 01/Jul/12 12:36 PM ]

Hi Mike,

Could you explain how you think the docstring definition implies this behavior? Maybe I'm missing something, but I was surprised. For instance, in the case of (range 3 9 0), if start were truly inclusive as the docstring says, the result would be (3), not ().

You mentioned that you think the infinite sequence of 0's is consistent and in keeping with the definition of range. I'm not sure I agree. (0,0] is an empty set of length zero, and [0,0) is an empty set of length zero.

You state that empty list only makes sense for start >= end, except this is not the current behavior. Could you help me understand what you believe the appropriate behavior would be in each of the following three cases? (range 0 10 0), (range 10 0 0), and (range 0 0 0)?

A few options to consider:
1) Fix the docstring to reflect the actual behavior of range.
2) Handle the case of (range 9 3 0) => (9 9 9 ...) to make it consistent with the behavior of (range 3 9 0) => ()
3) Stop allowing a step of zero altogether.

Editing to Add (Note: I don't think the way someone else did something is always the right way, just wanted to do some digging on how other people have handled this in the past):
http://docs.python.org/library/functions.html#range (0 step returns ValueError)
http://www2.tcl.tk/10797 (range returns empty list for a zero step)
http://www.scala-lang.org/api/2.7.7/scala/Range.html (zero step is not allowed)

Comment by Dimitrios Piliouras [ 05/Jul/12 4:13 PM ]

It does make sense NOT to allow a step of zero (at least to me)... I wasn't going to say anything about this but if other popular languages do not allow 0, then I guess it makes more sense than I originally gave myself credit for... However, if we want to be mathematically correct then the answer would be to return an infinte seq with the start value like below. After all, what is a step of 0? does it make any difference how many steps you take if with every step you cover 0 distance? obviously not...you start from x and you stay at x forever...we do have repeat for this sort of thing though...

(take 5 (range 3 9 0)) => (3 3 3 3 3)

+1 for not allowing 0 step

Comment by Mike Anderson [ 08/Jul/12 8:49 AM ]

@Devin quite simple: a lazy sequence of nums starting from x with step 0.0 until it reaches an end value of y (where y > x) is an infinite sequence of x.

Consider the case where start is 0 and end is infinity (the default), you would expect sequences to go as follows:

step +2 : 0 2 4 6 8....
step +1 : 0 1 2 3 4....
step +0 : 0 0 0 0 0....

It would be inconsistent to make 0 a special case, all of these are valid arithmetic progressions. And all of these are consistent with the current docstring.

If you make 0 a special case, then people will need to write special case code to handle it. Consider the code to create a multiplication table for example:

(for [x (range 10)]
(take 10 (range 0 Long/MAX_VALUE x)))

This works fine if you produce an infinite sequence of zeros for step 0, but fails if you create an empty list as a special case for step 0.

As a related issue, I'd personally also favour negative step sizes also producing an infinite sequence. If we don't want to allow this though, then at least the docstring should be changed to say "a lazy seq of non-decreasing nums" and a negative step should throw an error.

Comment by Devin Walters [ 09/Jul/12 7:09 PM ]

Carrying over a message from the clojure-dev list by Stuart Sierra:

  • I called the ticket a defect. Does that seem reasonable?

yes

  • Does anyone actually use the 0 step behavior in their programs?

not if they have any sense

  • Has anyone been bitten by this in the past?

not me

  • Is this behavior intentional for some historical reason I don't know about or understand?

I doubt it.

  • Has this been brought up before? I couldn't find any reference to it via Google.

Not that I know of.

  • Are there performance implications to my patch?

I doubt it. Lazy seqs are not ideal for high-performance code anyway.

  • Am I addressing a symptom rather than the problem?

I think the problem is that the result of `range` with a step of 0 was never specified. Don't assume that the tests are specifications. Many of the tests in Clojure were written by over-eager volunteers who defined the tests based on whatever the behavior happened to be. The only specification from the language designer is the docstring. By my reading, that means that `range` with a step of 0 should return an infinite seq of the start value, unless the start and end values are equal.

-S

Comment by Devin Walters [ 09/Jul/12 7:10 PM ]

Carrying over a message by Michal Marczyk:

With a negative step, the comparison is reversed, so that e.g.

(range 3 0 -1)

evaluates to

(3 2 1)

I think this is the useful behaviour; the docstring should perhaps be
adjusted to match it.

Agreed on zero step.

Cheers,
Michał

Comment by Devin Walters [ 20/Jul/12 5:10 PM ]

Adding a new patch after hearing comments. This patch makes (range 9 3 0) => (9 9 9 9 ...), (range 3 9 0) => (3 3 3 3 ...), and () will be returned when (= start end). Also updated the docstring.

Comment by Timothy Baldridge [ 27/Nov/12 3:01 PM ]

Marking as vetted

Comment by Timothy Baldridge [ 27/Nov/12 3:04 PM ]

Patch applies cleanly. We've discussed this issue to death (for as simple as it is). I think it's time to mark it as screened.

Comment by Timothy Baldridge [ 27/Nov/12 3:06 PM ]

For some reason I'm not allowed to edit the attachments list. When you apply the patch, please apply inconsistent_range_fix.diff as that is the most recent version of the fix.

Comment by Rich Hickey [ 09/Dec/12 6:44 AM ]

As someone who has to read these tickets, I'd really appreciate it if you could keep the description up to date and accurate, with examples of the current and intended behavior and the strategy to be used in fixing. I can't follow every thread to see what the latest thinking is, especially on a patch like this where the original mission was changed.

Thanks

Comment by Devin Walters [ 10/Dec/12 3:53 PM ]

@Tim: I've removed the other attachments.

@Rich: Understood. I will update the description this evening.





[CLJ-1011] clojure.data/diff should cope with null and false values in maps Created: 12/Jun/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Minor
Reporter: Philip Aston Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-clojure.data-diff-cope-with-falsey-values-in-maps.patch    
Patch: Code and Test
Approval: Ok

 Description   

Current behaviour of clojure.data/diff:

=> (diff {:a false} {:a true})
(nil {:a true} nil)
=> (diff {:a false} {:a nil})
(nil nil nil)

Proposed behaviour:

=> (diff {:a false} {:a true})
({:a false} {:a true} nil)
=> (diff {:a false} {:a nil})
({:a false} {:a nil} nil)


 Comments   
Comment by Philip Aston [ 12/Jun/12 5:04 AM ]
 
From e03a8060214d122ea2ebadf9e8a368f7f593d9f4 Mon Sep 17 00:00:00 2001
From: Philip Aston <philipa@mail.com>
Date: Sun, 10 Jun 2012 13:11:36 +0100
Subject: [PATCH] clojure.data/diff: cope with falsey values in maps

---
 src/clj/clojure/data.clj           |   18 +++++++++++++++++-
 test/clojure/test_clojure/data.clj |    3 ++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/clj/clojure/data.clj b/src/clj/clojure/data.clj
index 6e8dbcf..345b234 100644
--- a/src/clj/clojure/data.clj
+++ b/src/clj/clojure/data.clj
@@ -30,6 +30,22 @@
      (vec (repeat (apply max (keys m))  nil))
      m)))
 
+(defn- diff-associative-key
+  "Diff associative things a and b, comparing only the key k."
+  [a b k]
+  (let [va (get a k)
+        vb (get b k)
+        [a* b* ab] (diff va vb)
+        in-a (contains? a k)
+        in-b (contains? b k)
+        same (and in-a in-b
+                  (or (not (nil? ab))
+                      (and (nil? va) (nil? vb))))]
+    [(when (and in-a (or (not (nil? a*)) (not same))) {k a*})
+     (when (and in-b (or (not (nil? b*)) (not same))) {k b*})
+     (when same {k ab})
+     ]))
+
 (defn- diff-associative
   "Diff associative things a and b, comparing only keys in ks."
   [a b ks]
@@ -38,7 +54,7 @@
      (doall (map merge diff1 diff2)))
    [nil nil nil]
    (map
-    (fn [k] (map #(when % {k %}) (diff (get a k) (get b k))))
+    (partial diff-associative-key a b)
     ks)))
 
 (defn- diff-sequential
diff --git a/test/clojure/test_clojure/data.clj b/test/clojure/test_clojure/data.clj
index 9bab766..5a241e0 100644
--- a/test/clojure/test_clojure/data.clj
+++ b/test/clojure/test_clojure/data.clj
@@ -27,5 +27,6 @@
        [#{1} #{3} #{2}] (HashSet. [1 2]) (HashSet. [2 3])
        [nil nil [1 2]] [1 2] (into-array [1 2])
        [nil nil [1 2]] (into-array [1 2]) [1 2]
-       [{:a {:c [1]}} {:a {:c [0]}} {:a {:c [nil 2] :b 1}}] {:a {:b 1 :c [1 2]}} {:a {:b 1 :c [0 2]}}))
+       [{:a {:c [1]}} {:a {:c [0]}} {:a {:c [nil 2] :b 1}}] {:a {:b 1 :c [1 2]}} {:a {:b 1 :c [0 2]}}
+       [{:a nil} {:a false} {:b nil :c false}] {:a nil :b nil :c false} {:a false :b nil :c false}))
 
-- 
1.7.9.5
Comment by Andy Fingerhut [ 12/Jun/12 12:43 PM ]

Philip, thanks for writing that patch. Could you please attach it as a file to this ticket, instead of copying and pasting it into a comment? Instructions are under the heading "Adding patches" on this page:

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

Rich Hickey's policy is only to include code in Clojure that is written by those who have signed a contributor agreement. You can find out more at http://clojure.org/contributing

Comment by Philip Aston [ 12/Jun/12 2:51 PM ]

Thanks Andy. Patch attached:

0001-clojure.data-diff-cope-with-falsey-values-in-maps.patch

I'll send in a CA.

Comment by Philip Aston [ 16/Jun/12 5:27 AM ]

CA snail-mailed yesterday, I guess it will take a week to arrive.

Comment by Philip Aston [ 23/Jun/12 5:00 AM ]

I now have a CA in place.

Comment by Stuart Halloway [ 20/Jul/12 4:51 PM ]

Yeah, this should be fixed.

Comment by Aaron Bedra [ 14/Aug/12 9:42 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





[CLJ-990] Implement clojure.core.reducers/mapcat and some initial reducers tests Created: 10/May/12  Updated: 01/Mar/13  Resolved: 10/May/12

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, test

Attachments: Text File 0001-Implement-clojure.core.reducers-mapcat.patch     Text File 0002-Add-initial-reducers-tests.patch    
Patch: Code and Test

 Description   

The first patch implements a reducers equivalent of clojure.core/mapcat, and the second patch adds a new reducers.clj to the clojure tests that checks that map, mapcat, filter, and reduce are equivalent in clojure.core and clojure.core.reducers.



 Comments   
Comment by Rich Hickey [ 10/May/12 7:44 PM ]

applied - thanks!





[CLJ-987] pprint doesn't flush the underlying stream Created: 07/May/12  Updated: 01/Mar/13  Resolved: 14/Nov/12

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

Type: Defect Priority: Major
Reporter: David Powell Assignee: David Powell
Resolution: Completed Votes: 1
Labels: patch

Attachments: Text File 0002-pprint-now-flushes-the-underlying-stream-similarly-t.patch    
Patch: Code and Test
Approval: Ok

 Description   

Unlike prn, pprint doesn't flush the underlying stream.

pretty_writer currently overrides .flush with behaviour that pushes its own data, but does not flush the underlying stream.

pprint should behave similarly to prn with respect to flushing the underlying stream.



 Comments   
Comment by David Powell [ 07/May/12 9:43 AM ]

Test included.
Patch should ensure that flushing happens after pprint, without introducing any additional unnecessary flushes.

Comment by Stuart Halloway [ 08/Jun/12 3:11 PM ]

I am confused by the tests – they all seem to call prn, though some claim to be calling pprint.

Comment by David Powell [ 09/Jun/12 6:16 AM ]

Ah, sorry. I'd tried to remove duplicate tests before committing them, but removed the wrong ones.
Replacement patch attached:

0002-pprint-now-flushes-the-underlying-stream-similarly-t.patch

Comment by Andy Fingerhut [ 21/Jun/12 5:27 PM ]

Tempting fate once again by changing Approval from Incomplete to None after the reason it was marked as incomplete seems to have been addressed.

Comment by Stuart Sierra [ 09/Nov/12 4:07 PM ]

Screened.

Comment by Stuart Sierra [ 14/Nov/12 9:23 AM ]

Pushed in commit 1588ff3f70e864d9817bc565bd2c30b08189bc6e





[CLJ-982] Implement an interface? predicate to balance class? Created: 04/May/12  Updated: 09/Jun/12  Resolved: 08/Jun/12

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

Type: Enhancement Priority: Trivial
Reporter: David Rupp Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch, test
Environment:

Any


Attachments: File drupp-add-interface-predicate.diff    
Patch: Code and Test

 Description   

clojure.core implements a class? predicate to detect if a thing is an instance of java.lang.Class. The attached patch implements interface? to further distinguish classes that are also interfaces. This gives us Clojure api for e.g., enumerating all interfaces a thing's base class implements; as in, (filter #(interface? %) (supers (class my-thing))).



 Comments   
Comment by Stuart Halloway [ 08/Jun/12 12:28 PM ]

I would prefer to see this, and other interop/reflective items, in a separate contrib lib.

Comment by David Rupp [ 09/Jun/12 9:08 AM ]

Thanks for the feedback, Stu. Is there an existing contrib lib for stuff like this? Or should I create one?





[CLJ-960] Capture :column metadata (needed for ClojureScript source maps) Created: 27/Mar/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: patch

Attachments: File CLJ-960-tests.diff     Text File columns-1.patch     Text File columns-1-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

I've begun working on implementing SourceMaps for ClojureScript. For an overview of SourceMaps, see: http://www.html5rocks.com/en/tutorials/developertools/sourcemaps/ For discussion of the feature in ClojureScript, see: https://groups.google.com/d/topic/clojure-dev/zgmXO2iM1JQ/discussion

In order to produce accurate source maps, I need column information in addition to line information from the Clojure reader.

I've made the necessary enhancement to LispReader, etc. but have some cleanup and testing left to do. I'd also like a sanity check from the core team before attaching a formal patch. You can find my work in progress here: https://github.com/brandonbloom/clojure/compare/columns



 Comments   
Comment by David Nolen [ 13/Apr/12 8:29 AM ]

You need to attach a patch so this can be assessed. Thanks!

Comment by Brandon Bloom [ 31/May/12 6:09 PM ]

Sorry David! I added a patch a while ago but forgot to add the patch label(s?) as well as leave a comment.

The patch may be out of date now, but I haven't checked. Hopefully the prescreening process will pick it up automatically run the tests.

Comment by Andy Fingerhut [ 31/May/12 6:32 PM ]

Yes, Brandon, your patch has been on the list of prescreened patches since April 15. It has always applied and built cleanly during that time. That patch label is nice to have for certain JIRA report filters, but isn't necessary for the prescreening process to pick it up.

Comment by John Szakmeister [ 27/Jul/12 5:32 AM ]

v2 now applies against the current master (191b05f1). The original patch seemed to be broken from a whitespace perspective, which was making it difficult to apply--even in a 3-way merge. The only real conflict was in Compiler.java where a "final int line" was added to CompilerException. All the tests passed.

Comment by Brandon Bloom [ 27/Jul/12 7:33 PM ]

It looks like the line field was added to CompilerException in commit 89245c68, but that commit doesn't use it for anything. Maybe a later commit uses it? Also, if we want to keep that field, can we also add a column field for patch v3?

Comment by David Nolen [ 27/Jul/12 7:36 PM ]

This patch is an enhancement. In order for this one to make any movement I believe it will need a design page outlining the problem, what this solves / alternatives.

Comment by Brandon Bloom [ 25/Aug/12 9:38 PM ]

http://dev.clojure.org/display/design/Compiler+tracking+of+columns

I added a design page, but it seems like overkill. This is a straightforward enhancement...

Comment by Rich Hickey [ 04/Oct/12 8:51 AM ]

I've applied the v2 patch (thanks!), but before we close the ticket can we please get a patch comprising some tests?

Comment by Chas Emerick [ 19/Oct/12 11:45 AM ]

Attached CLJ-960-tests.diff to verify :line and :column metadata as now provided by LineNumberingPushbackReader.

Comment by Stuart Halloway [ 19/Oct/12 3:02 PM ]

The tests are a little fragile (exact map comparison) and so will break if the reader ever does more things. In library and app projects I write tests like this using core.match, but that isn't available as a build dependency here.

Marking screened anyway.





[ASYNC-59] Channel returned by cljs.core.async/map> is missing protocol method Channel.closed? Created: 08/Mar/14  Updated: 10/Sep/14  Resolved: 02/Sep/14

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Kevin Neaton Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, patch
Environment:

[org.clojure/clojure "1.5.1"]
[org.clojure/clojurescript "0.0-2173"]
[org.clojure/core.async "0.1.278.0-76b25b-alpha"]


Attachments: Text File 0001-Fixed-map-by-including-impl.-for-closed.patch    
Patch: Code

 Description   

E.g.

(let [ch (->> (chan) (map> inc) (filter> even?))]
  (doseq [i (range 10)] (put! ch i)))

When filter> checks to see if the channel returned by map> is closed?, this code fails because the channel returned by map> does not implement the Channel.closed? protocol method.



 Comments   
Comment by Alex Miller [ 02/Sep/14 9:54 AM ]

map> and filter> have been deprecated and will be removed in a future release. They have been replaced with applying transducers to a channel which is now available.

Comment by Kevin Neaton [ 10/Sep/14 9:08 AM ]

Great, thanks for the update.





[ALGOM-11] Add the reader monad Created: 05/Feb/14  Updated: 05/Feb/14  Resolved: 05/Feb/14

Status: Closed
Project: algo.monads
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Leonardo Borges Assignee: Konrad Hinsen
Resolution: Completed Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-Add-the-reader-monad-and-associated-tests.patch    
Patch: Code and Test

 Description   

This patch adds the reader monad and supporting functions. It includes tests.

I've been using it with my own patched version of algo.monads. It would be nice to be able to
rely on the original project for the dependency as opposed to depending on a local built version.



 Comments   
Comment by Konrad Hinsen [ 05/Feb/14 5:11 AM ]

Done: http://github.com/clojure/algo.monads/commit/3055d0dc19d0e497556b5d6dc86c77a112d2dcd2





Generated at Sat Sep 20 07:14:49 CDT 2014 using JIRA 4.4#649-r158309.