<< Back to previous view

[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-3] BigDecimal: unnecessary reflection used in math functions for coercion (bigint) Created: 13/May/13  Updated: 02/Jan/14  Resolved: 02/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Jürgen Hötzel Assignee: Mark Engelberg
Resolution: Declined Votes: 0
Labels: patch,

Attachments: Text File 0001-Use-coercion-and-rounding-methods-provided-by-BigDec.patch    
Patch: Code

 Description   

This improves performance, because no reflection has to be used (bigint):

Patched:
clojure.math.numeric-tower> (time (let [b (BigDecimal. "2.2")] (dotimes [x 10000000] (round b))))
"Elapsed time: 1640.177041 msecs"
Orig:
clojure.math.numeric-tower> (time (let [b (BigDecimal. "2.2")] (dotimes [x 10000000] (round b))))
"Elapsed time: 3498.339271 msecs"



 Comments   
Comment by Mark Engelberg [ 02/Jan/14 12:36 AM ]

Andy, thanks for letting me know about this issue. I had no idea it's been sitting here in JIRA for months.

1. These numbers really do need to be converted to BigInt, not BigInteger.

2. There's nothing inherently problematic with the call to bigint. In my tests, it appears that this patch doesn't meaningfully improve the performance of floor or ceiling, only round. That's because it's not the call to bigint that is slow, but in round, you've used a call to .setScale with ROUND_HALF_UP, and that's faster than the method used in the code (adding 0.5M and then flooring).

3. Even though .setScale with ROUND_HALF_UP is faster than the current code, it does not return the correct results for negative numbers like -4.5M. The goal is to make the behavior match Math/round on doubles. For some reason, BigDecimal's ROUND_HALF_UP does the wrong thing for negative numbers (rounding up in magnitude, rather than rounding up in the sense of making it more positive).

So this patch is a no-go. The changes to floor and ceiling return BigInteger rather than BigInt and when modified to return BigInt, do not yield speed benefits. The change to round does yield a speed benefit, but the results are incorrect for negative numbers halfway between integers.

If you find a faster way to round decimal numbers than the existing mechanism that yields the correct results for negative numbers, let me know.





[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





[JDBC-31] distinct? throws clojure.lang.ArityException, when applied with no arguments Created: 12/May/12  Updated: 10/Jun/12  Resolved: 10/Jun/12

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

Type: Defect Priority: Major
Reporter: Jürgen Hötzel Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: patch,, performance

Attachments: Text File 0001-add-make-cols-unique-test-case-empty-cols.patch     Text File 0002-distinct-throws-clojure.lang.ArityException-when-app.patch    
Patch: Code and Test

 Description   

HSQLDB returns an empty ResultSet when using (.getGeneratedKeys stmt)
and no keys are generated. So this Exception is thrown for each record
without generated keys.

While this Exception is caught in do-prepared-return-keys, this can lead to a huge overhead caused by the JVM exception handling. I did a performance test.

Before Patch:

clojure.java.test-jdbc> (time (sql/with-connection hsqldb-db (count (apply sql/insert-records :dummy  (map #(hash-map :name (str %) :id %) (range 10000))))))
"Elapsed time: 3429.346743 msecs"
10000

After Patch:

 clojure.java.test-jdbc> (time (sql/with-connection hsqldb-db (count (apply sql/insert-records :dummy  (map #(hash-map :name (str %) :id %) (range 10000))))))
"Elapsed time: 1397.444753 msecs"


 Comments   
Comment by Sean Corfield [ 10/Jun/12 5:24 PM ]

Thanx for the patch!





[JDBC-29] Performance improvement (Remove intermediate lazy sequence in resultset-seq) Created: 10/May/12  Updated: 10/May/12  Resolved: 10/May/12

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

Type: Enhancement Priority: Minor
Reporter: Jürgen Hötzel Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: patch,, performance

Attachments: Text File 0001-Don-t-create-intermediate-lazy-sequences-of-vectors.patch    

 Description   

This improves performance on large result sets by up to 30%.



 Comments   
Comment by Sean Corfield [ 10/May/12 12:09 PM ]

Nice catch!





[DXML-1] Stack overflow when parsing huge XML file Created: 10/Feb/12  Updated: 20/Mar/12  Resolved: 20/Mar/12

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

Type: Defect Priority: Major
Reporter: Justin Kramer Assignee: Ryan Senior
Resolution: Completed Votes: 1
Labels: patch,
Environment:

OS X


Attachments: Text File data-xml-kwopts.patch    
Patch: Code and Test

 Description   

This is using Ryan Senior's new 0.0.3-SNAPSHOT.

While trying to parse a huge XML file (7.5 GB compressed, a dump of Wikipedia), got a stack overflow error. Some digging turned up this bug:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6440214

Modifying clojure.data.xml/source-seq to disable the IS_COALESCING property got rid of the error.

The old lazy-xml contrib code worked (although used up tons more memory).

Attached is a patch that adds keyword options to source-seq, parse, and parse-str, allowing the consumer to disable coalescing and sidestep the upstream bug.



 Comments   
Comment by Ryan Senior [ 20/Mar/12 8:05 AM ]

Thanks Justin!





[CLJS-740] undeclared-ns warnings are broken Created: 02/Jan/14  Updated: 17/Jan/14  Resolved: 17/Jan/14

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

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

Attachments: File fix-undeclared-ns-warnings.diff    
Patch: Code

 Description   

In the recent versions of cljs, the undeclared-ns warnings have stopped working. This patch seems to be the culprit.



 Comments   
Comment by David Nolen [ 02/Jan/14 12:58 PM ]

Great thanks!

Comment by David Nolen [ 07/Jan/14 10:32 AM ]

CLJS-615

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

fixed, https://github.com/clojure/clojurescript/commit/b8cf302b1500e88e0602e72fa6aec6f7328a1a00





[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-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-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-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-387] Add docstring from def and ns definitions to @namespaces metadata map, and make reflect functions make use of that Created: 07/Oct/12  Updated: 27/Jul/13  Resolved: 17/Oct/12

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

Type: Enhancement Priority: Minor
Reporter: Frank Siebenlist Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docs, enhancement, patch,, reflection
Environment:

clojure/clojurescript "0.0-1450"


Attachments: File add-ns-def-doc-patch.diff    
Patch: Code

 Description   

The docstrings were parsed from the definitions-forms for def and ns, but not added to the @namespaces metadata map.
There is no :doc entry used in the ns' metadata in the @namespaces.
No ns's :doc info is communicated to the browser in the reflect functions with reflect/doc.
Patch-file is attached with code-changes that add the :doc info for ns and def to the @namespaces, and enhances the reflect functions to communicate that info to the browser in the reflect/doc call.



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

This patch no longer applies, mind updating it?

Comment by Frank Siebenlist [ 16/Oct/12 12:10 AM ]

This patch should apply to master version on Mon, 15 Oct 2012 22:03:19 -0700 (4defcbcf19112b9be6a4a27b5d8855552bf94948)

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

Excellent, fixed http://github.com/clojure/clojurescript/commit/bef56a74f2eeecabfe0c0a28d89b455dce576ea3

Please at the ticket # to the commit message though, thanks!





[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





[CLJS-260] Add clojure.core/shuffle implementation Created: 17/May/12  Updated: 27/Jul/13  Resolved: 20/May/12

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

Type: Enhancement Priority: Minor
Reporter: Evan Mezeske Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch,, test

Attachments: Text File 0001-Add-clojure.core-shuffle-and-a-test.patch     Text File shuffle.patch     Text File shuffle.v2.patch    
Patch: Code and Test

 Description   

I added a simple implementation of clojure.core/shuffle, which uses goog.array's Fisher-Yates for its implementation. Included in the patch is a test to make sure it works.



 Comments   
Comment by David Nolen [ 17/May/12 7:07 PM ]

Why don't you return a vector like Clojure does?

Comment by Evan Mezeske [ 17/May/12 7:11 PM ]

Return a vector like Clojure.

Comment by David Nolen [ 18/May/12 9:23 PM ]

The patch is not correctly formatted with attribution.

Comment by Evan Mezeske [ 20/May/12 4:24 PM ]

Use git format-patch instead of git diff, to hopefully provide the right patch format.

Comment by David Nolen [ 20/May/12 10:28 PM ]

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





[CLJS-215] js-obj function broken Created: 29/Apr/12  Updated: 27/Jul/13  Resolved: 29/Apr/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,

Attachments: Text File js-obj-fn.patch    
Patch: Code and Test

 Description   

The js-obj macro is fine, but the js-obj function was incorrectly using "{foo: bar}" expecting "foo" to be evaluated as a variable, instead of as a string key.

Since we have the js-obj macro for speed, I just deleted the broken overloads, so now all the non-zero arity cases simply call gobject



 Comments   
Comment by David Nolen [ 29/Apr/12 5:08 PM ]

fixed, https://github.com/clojure/clojurescript/commit/f4a3ef43dc70aed4f15045836d709989a95ef6f7





[CLJS-211] apply does not work on IFn objects in CLJS Created: 27/Apr/12  Updated: 27/Jul/13  Resolved: 28/Apr/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,

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

 Description   

http://dev.clojure.org/jira/browse/CLJS-135 fixed this for multimethods, but the same issue applies to anyone who implements IFn.

The attached patch fixes this by modifying extend-type to emit an apply method on the extended object's prototype.



 Comments   
Comment by Brandon Bloom [ 28/Apr/12 2:31 AM ]

Updated with patch!

Worth noting: String has an implementation of IFn and manually defines prototype.apply. I contemplated deleting it, since this makes it unnecessary, however, my general purpose implementation is probably a tad slower. Maybe, maybe not. Depends on how Google's inlining does. I figured: make it work, then make it fast. So I didn't bother profiling them and left the existing implementation there.

Comment by David Nolen [ 28/Apr/12 12:50 PM ]

fixed, https://github.com/clojure/clojurescript/commit/ddd8199b324997cfe0537be66372162b41ce8293





[CLJS-210] Implement Var form, var-get, and var? in CLJS Created: 27/Apr/12  Updated: 28/Oct/12

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

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

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

 Description   

See discussion of Vars in CLJS here: http://dev.clojure.org/display/design/Dynamic+Binding
My goal is to eventually implement dynamic scope capture, via bound-fn and friends. The attached patch is a step in that direction.

This patch adds support for parsing the (var foo) form. The #' reader macro is provided by JVM Clojure.

#'foo emits code to construct a Var object. In this patch, each invocation of 'var will create a unique Var object. This means they are '= by fully qualified symbol, but not 'identical?. Simple memoization would fix that, but I'm not going to bother until I get to Dynamic Var objects.

The main advantage of this level of Var support is for the interactive development convenience of being able to defer dereferencing top-levels. For example, (def fog (comp f #'g)) will pick up redefinitions of 'g, but not of 'f.



 Comments   
Comment by Brandon Bloom [ 28/Apr/12 10:07 PM ]

I found two issues with this patch:

1) I never used the IVar that I declared :-P

2) (apply #'+ (range 100)) throws "Invalid arity: 101" – this is related to http://dev.clojure.org/jira/browse/CLJS-211

I'll look into fixing both. However, we should discuss supporting variable arity protocol methods...

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

any support for reified vars is a low priority for the foreseeable future.

Comment by Brandon Bloom [ 01/Sep/12 7:46 PM ]

Reified vars (and binding frames!) are a requirement for a CPS transform to be correct with respect to dynamic bindings.

Comment by Tom Jack [ 10/Sep/12 5:11 PM ]

+1 towards bound-fn and friends. I had an explanation written here but now I see "async Javascript (ie. nearly all Javascript) makes the binding macro effectively useless without bound-fn". Indeed.

Comment by Herwig Hochleitner [ 28/Oct/12 7:34 PM ]

bound-fn could also be implemented by setting / resetting bound vars before / after the wrapped fn, similar to binding. We would just need to add tracking of the currently rebound dynamic vars.
That should also be CPS - friendly, no?

Comment by Tom Jack [ 28/Oct/12 9:03 PM ]

I've considered that and would love to see it happen.

But what about:

(def ^:dynamic *foo* 3)
(set! *foo* 4)
(def f (bound-fn [] *foo*))
(set! *foo* 5)
(f)




[CLJS-177] js->clj should convert JavaScript null to Clojure nil Created: 10/Apr/12  Updated: 27/Jul/13  Resolved: 10/Apr/12

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

Type: Defect Priority: Minor
Reporter: Jason Rudolph Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch,

Attachments: File js-null-to-clj-nil.diff    
Patch: Code and Test

 Description   

Prior to commit ebf45f5 [A], js->clj converted JavaScript null to Clojure nil without any errors.

As of commit ebf45f5 (related to CLJS-175), js->clj throws a TypeError when it encounters JavaScript null:

TypeError: Cannot read property 'constructor' of null
b===c}function q(b,c){return gb.call(e,b,c)}function pb(b){return b.construct
^
TypeError: Cannot read property 'constructor' of null

The attached patch [B] adds a test to demonstrate the issue and provides a suggested fix.

Note: CLJS-175 intentionally limited the types of objects it would attempt to convert to Clojure code. For example, CLJS-175 notes that conversion "doesn't make sense for something like Date or RegExp." I agree for those types of objects, but "null" seems straightforward enough that it should be converted.

[A] https://github.com/clojure/clojurescript/commit/ebf45f5
[B] Also viewable here: https://github.com/jasonrudolph/clojurescript/compare/master...js-null-to-clj-nil



 Comments   
Comment by David Nolen [ 10/Apr/12 7:38 PM ]

Fixed, http://github.com/clojure/clojurescript/commit/9319579acfc4fc9dbcf6e79f611afad707f97579





[CLJS-176] Track line & column in generated JS (needed for SourceMaps) Created: 09/Apr/12  Updated: 27/Jul/13  Resolved: 15/Apr/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,

Attachments: File track-pos-1.diff     Text File track-pos-2.patch     Text File uninit-var-print.patch    
Patch: Code

 Description   

The attached patch modifies the ClojureScript compiler by replacing all calls to print, println, emits, etc, such that all code generation funnels through the new emitx and emitln. The emitx and emitln functions track position as a 2-tuple vector of [line, column], zero-based. The line and column information is a prerequisite for generating correct source maps. NOTE: This is the position in the generated JavaScript source, not the :line and :column metadata from the Clojure reader.

In the emitln function, there are a few commented out lines which constitute a "test". Basically, it causes the compiler to append a line number // comment on each line of the generated source. The comments are horizontally aligned at 120 characters, so you can quickly visually inspect for alignment bugs. Unfortunately, I'm not really sure how better to test this, or to ensure that this keeps working until the rest of SourceMaps gets implemented In theory, as long as no one prints anything directly, always using emitx and emitln, nothing should break.



 Comments   
Comment by Brandon Bloom [ 12/Apr/12 12:03 AM ]

This patch is now out of date. I'd be happy to update it, but the compiler moves fast enough that I'll wait for general feedback. I don't want to have to constantly re-update this patch.

Comment by David Nolen [ 12/Apr/12 11:03 AM ]

emitx is a little too ambiguous for my tastes. What about emit-ln and emit-str? Other options? Once we decide on names I'd be happy to merge this in quickly.

Comment by Brandon Bloom [ 12/Apr/12 1:39 PM ]

This patch removes a function that was called emits (note the "s"). It simply called emit inside a with-out-string.

The emitx function in this patch isn't that simple. It does some special handling of nil, maps, seqs, functions, etc. It creates a mini DSL for emitting code, so that I didn't have to drastically rewrite all the places that used (print (emit-str ...)) and similar before.

I decided not to call it emit-str to avoid confusion with the old emits function, as well as to differentiate the behavior from prn-str and the others, which return a string without affecting the previously set out. Quite honestly, the best name for it is simply "emit", but that honor goes to the current multi-method, which is probably best named "emit-ast". I don't think it's worth the churn to rename that.

Either way, I'm not too picky. If you have a preferred name, I'll update the patch to whatever you like.

Comment by David Nolen [ 12/Apr/12 2:42 PM ]

It's a big patch, it's not clear to me why you need emitx to handle all those cases. Can you point out specific lines? Thanks.

Comment by Brandon Bloom [ 12/Apr/12 5:50 PM ]

Any line that used to use 'emits was potentially problematic because it allowed things to be printed out of order. Consider this:

(let [xyz (with-out-str (print-mystery))]
(println "abc")
(println xyz)
(println "123"))

How many newlines were printed? At what column is "123" printed? Without knowing the contents of 'xyz, there is no way to know. You could split by occurrences of
n in 'xyz, but that would negatively affect performance and doesn't alleviate the need to funnel all printing through a single choke point.

Instead, I opted to restructure to something like this:

(let xyz #(emit-mystery)
(emitln "abc")
(xyz)
(emitln "123"))

So that's why 'emitx handles 'fn? and the :else case. 'map? is for handling the ast 'emit multi-method. 'nil? and 'seq? are convinces for common cases. The behavior is similar to expansion of seqs in Hiccup. There are numerous examples of each.

Comment by David Nolen [ 13/Apr/12 8:23 AM ]

K I understand that patch quite a bit better now. Makes sense. I still think emitx is not a great name. Lets stick with emits.

Comment by Brandon Bloom [ 13/Apr/12 2:16 PM ]

I just looked through the source and renaming 'emit to 'emit-ast and 'emitx to 'emit wouldn't actually be that much churn. There are only one or two references to 'emit outside of the 'defmethod calls. Also, anyone who calls 'emit, expecting 'emit-ast, will still get the right result. I'll go with that, unless you feel strongly about 'emits.

Hopefully, I'll find time to update the patch this weekend.

Is there a resource regarding testing the compiler? Wiki page or something? I've been diffing the output of the Twitter sample, but it would be nice to have a more rigorous test harness.

Comment by Brandon Bloom [ 13/Apr/12 2:29 PM ]

Sorry, shouldn't write comments before I have my coffee

I know about this page: https://github.com/clojure/clojurescript/wiki/Running-the-tests

But that page mentions "make sure the repl hasn't broken". I'm assuming that the test coverage for the compiler isn't good enough to accept patches without additional manual testing. Is that a correct assumption? What's your test process? I'd like to minimize the effort for you to accept patches in the future.

Comment by David Nolen [ 13/Apr/12 3:01 PM ]

I see no compelling reason to switch to emit-ast. Lets just stick with emit, emits, emitln please.

As far as testing, I normally just test using ./script/test. Issues w/ Browser REPL tend to crop up if someone is messing with closure.clj or the REPL source itself. It's simple to test and I don't mind doing that.

Comment by Brandon Bloom [ 14/Apr/12 7:49 PM ]

Updated with patch 2. Please don't make me merge more optimizations

Comment by Brandon Bloom [ 14/Apr/12 7:54 PM ]

You want attachment 11052, not 11051

Comment by David Nolen [ 15/Apr/12 2:12 PM ]

Fixed, https://github.com/clojure/clojurescript/commit/e4c6ae4e0639efaa648efa351c366a2f686be51e

Comment by Brandon Bloom [ 16/Apr/12 11:52 PM ]

Apparently I missed one println call on line 443. I've attached uninit-var-print.patch to correct it.

I hope you see updates to this issue, even though it's resolved. It seems really heavyweight to use a JIRA ticket for such a trivial change. Let me know if I really should have.

Comment by David Nolen [ 17/Apr/12 12:57 PM ]

Fixed in master, thanks.





[CLJS-169] Forward slashes should not be escaped in javascript strings Created: 25/Mar/12  Updated: 27/Jul/13  Resolved: 30/Mar/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,

Attachments: File no-escape-slash.diff    
Patch: Code

 Description   

cljs.compiler/escape-char incorrectly escapes forward slashes.

Generally, this isn't a problem because most Javascript engines treat unknown escape sequences as non-escape sequences. However, I ran into a JSON parser that barfs on it while working on source-maps. I tried to use escape-string for producing the JSON-based source-map by reusing the existing compiler functions for emitting valid Javascript.



 Comments   
Comment by David Nolen [ 30/Mar/12 3:21 PM ]

fixed, https://github.com/clojure/clojurescript/commit/29d60c7eb2c79768e2e2a2c572a4e4eff132946d





[CLJS-164] Add missing core printing functions Created: 18/Mar/12  Updated: 27/Jul/13  Resolved: 30/Mar/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,

Attachments: File pr-fns.diff     File pr-fns-with-tests.diff    
Patch: Code

 Description   

I've attached a patch which adds a few missing printing functions to ClojureScript core.

New functions:

  • prn-str
  • print-str
  • println-str


 Comments   
Comment by David Nolen [ 19/Mar/12 12:14 PM ]

Mind adding the related tests?

Comment by Brandon Bloom [ 25/Mar/12 11:16 PM ]

New diff with tests.

Sorry I didn't get to these sooner. I'm set as "watching" this issue, but I didn't get emailed when you commented.

Comment by David Nolen [ 30/Mar/12 3:20 PM ]

fixed, https://github.com/clojure/clojurescript/commit/3a3d9b1e3ab8858f26711a8860f48619736eba2d





[CLJ-1084] (object-array [1]) is ~3x slower than (object-array (rseq [1])) Created: 09/Oct/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: Paul Stadig Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: patch,

Attachments: Text File 0001-Make-PersistentVector-ChunkedSeq-implement-Counted.patch     Text File CLJ-1084-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

{{user=> (time (dotimes [_ 10000000] (object-array [1])))
"Elapsed time: 1178.116257 msecs"
nil
user=> (time (dotimes [_ 10000000] (object-array (rseq [1]))))
"Elapsed time: 422.42248 msecs"
nil}}

This appears to be because PersistentVector$ChunkedSeq does not implement Counted, so RT.count is iterating the ChunkedSeq to get its count.



 Comments   
Comment by Paul Stadig [ 09/Oct/12 2:11 PM ]

I don't believe this is Major priority, but I cannot edit the ticket after having created it.

Comment by Tassilo Horn [ 11/Oct/12 10:17 AM ]

This patch makes PersistentVector$ChunkedSeq implement Counted.

Performance before:

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.862259 msecs"
;"Elapsed time: 7228.72486 msecs"

Performance after (with the patch):

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.967301 msecs"
;"Elapsed time: 0.99391 msecs"

Also with Paul's test case.

Before:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 1668.346997 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 662.820591 msecs"

After:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 757.084577 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 680.602921 msecs"
Comment by Stuart Halloway [ 19/Oct/12 1:46 PM ]

Two patches to be applied together, the 10/19 patch adds tests and updates to latest test.generative.





[CLJ-963] Support pretty printing namespace declarations under code-dispatch Created: 29/Mar/12  Updated: 01/Sep/12  Resolved: 01/Sep/12

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

Type: Defect Priority: Minor
Reporter: Tom Faulhaber Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: patch,
Environment:

all


Attachments: File pprint-ns-patch.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

Currently when you print code with an (ns ) declaration in it, the ns declaration comes out kind of messy. This patch makes that beautiful.



 Comments   
Comment by Stuart Sierra [ 24/Aug/12 8:30 AM ]

Screened.

It's not perfect, but an improvement.

Before the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns
  com.some-example.my-app
  (:require
    clojure.string
    [clojure.set :as set]
    [clojure.java.io :refer (file reader)]))
nil

After the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns com.some-example.my-app
  (:require clojure.string [clojure.set :as set]
            [clojure.java.io :refer (file reader)]))
nil




[CLJ-953] drop-while doc string wrong, nil instead of logical false Created: 15/Mar/12  Updated: 30/Mar/12  Resolved: 30/Mar/12

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

Type: Enhancement Priority: Trivial
Reporter: Eric Schoonover Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: documentation, patch,

Attachments: File take-while_doc_string_CLJ-953.diff    
Patch: Code
Approval: Ok

 Description   
"Returns a lazy sequence of the items in coll starting from the first
-  item for which (pred item) returns nil."
+  item for which (pred item) returns logical false."





[CLJ-886] java.io/do-copy can garble multibyte characters Created: 28/Nov/11  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Defect Priority: Major
Reporter: Jeff Palmucci Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: patch,
Environment:

all


Attachments: File clj-886.diff     Text File CLJ-886-fix2.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

See comments in fix:

(defmethod do-copy [InputStream Writer] [#^InputStream input #^Writer output opts]
;; WRONG! if the buffer boundry falls in the middle of a multibyte character, we will get garbled results.
#_
(let [#^"[B" buffer (make-array Byte/TYPE (buffer-size opts))]
(loop []
(let [size (.read input buffer)]
(when (pos? size)
(let [chars (.toCharArray (String. buffer 0 size (encoding opts)))]
(do (.write output chars)
(recur)))))))
;; here we decode the characters before stuffing them into the buffer
(let [#^"[C" buffer (make-array Character/TYPE (buffer-size opts))
in (InputStreamReader. input (encoding opts))]
(loop []
(let [size (.read in buffer 0 (alength buffer))]
(if (pos? size)
(do (.write output buffer 0 size)
(recur)))))))



 Comments   
Comment by Jeff Palmucci [ 05/Dec/11 11:23 AM ]

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

Comment by Jeff Palmucci [ 19/Dec/11 8:53 AM ]

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

Comment by Andy Fingerhut [ 09/Feb/12 8:13 PM ]

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

Comment by Jeff Palmucci [ 13/Feb/12 7:29 AM ]

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

Comment by Stuart Sierra [ 17/Feb/12 2:55 PM ]

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





[CLJ-879] Allow :require to support a :refer clause Created: 17/Nov/11  Updated: 17/Feb/12  Resolved: 17/Feb/12

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

Type: Enhancement Priority: Minor
Reporter: Phil Hagelberg Assignee: Phil Hagelberg
Resolution: Completed Votes: 9
Labels: patch,, test

Attachments: Text File 0001-Allow-require-to-take-a-refer-option.patch    
Patch: Code and Test
Approval: Ok

 Description   

There's been discussion previously about the complexity of the ns
macro. In particular the fact that :use causes all vars to be referred
by default is widely seen as unfortunate, and the distinction between
use and require is a source of conceptual overhead for people new to
the language.

We can't change the fact that :use refers everything by default
without breaking lots of existing code. But it would be possible to
enhance :require to support referring specified vars, leaving us free
to deprecate or otherwise discourage the use of :use.

Clojure-dev thread discussing this: http://groups.google.com/group/clojure-dev/browse_thread/thread/91b708ddb909affd



 Comments   
Comment by Phil Hagelberg [ 25/Nov/11 2:52 PM ]

Patch containing implementation, test, and documentation.

Comment by Phil Hagelberg [ 06/Jan/12 2:21 PM ]

Any chance we could get some discussion going on this?

Comment by Devin Torres [ 14/Jan/12 1:12 AM ]

I'd love to see this discussed.

Comment by Phil Hagelberg [ 27/Jan/12 4:42 PM ]

So... how about it? Thoughts?

Comment by Kevin Downey [ 17/Feb/12 11:30 AM ]

patch still applies cleanly to master, tests pass

there is a warning when you run the tests though:

[java] Testing clojure.test-clojure.keywords
[java]
[java] Testing clojure.test-clojure.load
[java] WARNING: with-bindings already refers to: #'clojure.core/with-bindings in namespace: clojure.test-clojure.require-scratch, being replaced by: #'clojure.main/with-bindings
[java]
[java] Testing clojure.test-clojure.logic
[java]
[java] Testing clojure.test-clojure.macros

Comment by Stuart Sierra [ 17/Feb/12 1:36 PM ]

Vetted. Patch is good, although it needs docstring updates in 'require'

Comment by Stuart Sierra [ 17/Feb/12 1:55 PM ]

Not Vetted. I can't remember what the names mean. Ready for Rich. That's "Test" right?

Comment by Stuart Sierra [ 17/Feb/12 1:59 PM ]

My mistake. The docstring is included in 'require', but not in 'refer', which also changed. But the public API of 'refer' did not change. So this patch is good.

Comment by Stuart Sierra [ 17/Feb/12 2:08 PM ]

Not "Test." Screened. The patch is Screened. I hate this.

Comment by Stuart Sierra [ 17/Feb/12 2:18 PM ]

Patch applied.





[CCACHE-22] Change limit parameter name in TTLCache Created: 30/Mar/12  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Enhancement Priority: Trivial
Reporter: Sebastián Bernardo Galkin Assignee: Fogus
Resolution: Completed Votes: 0
Labels: enhancement, patch,, ttl

Attachments: Text File Changed-limit-parameter-name-in-TTLCache.patch    
Patch: Code

 Description   

"limit" is used in other caches to represent quantity, for TTL it represents time. Using the same name is confusing



 Comments   
Comment by Fogus [ 15/Jun/12 1:48 PM ]

Changed to :ttl-ms





Generated at Sat Apr 19 09:31:24 CDT 2014 using JIRA 4.4#649-r158309.