<< Back to previous view

[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





Generated at Thu Aug 28 22:27:05 CDT 2014 using JIRA 4.4#649-r158309.