Issue 383: Remove unused let-bindings from the generated RHS activati… #384
Conversation
There was a problem hiding this comment.
I had comments on the issue about some of the discussion points
#383 (comment)
Then a few comments on this PR itself.
| assignments (sequence | ||
| (comp | ||
| (filter rhs-bindings-used) | ||
| (mapcat #(list (symbol (name %)) (list 'get-in '?__token__ [:bindings %])))) |
There was a problem hiding this comment.
It isn't really part of the change you are making here, but 2 things:
get-in shouldn't be left unqualified. There is no good reason to not ensure it is clojure.core/get-in instead of perhaps an overridden var with that same name in the compilation ns.
That said, I think using get-in here at all is unnecessarily slow. This is a 2 keyword lookup and should be done in a more direct manner.
(mapcat #(list (symbol (name %)) (-> '?__token__ :bindings %)))There was a problem hiding this comment.
+1 to both points. I will make the change.
| @@ -309,8 +315,7 @@ | |||
| "Returns a set of the symbols in the given s-expression that start with '?' as keywords" | |||
| [expression] | |||
| (into #{} (for [item (flatten-expression expression) | |||
| :when (and (symbol? item) | |||
| ;; away are in fact optimized away. The target here is not the actual execution time, | ||
| ;; which will vary per system, but verification that the action operations in question are not performed. | ||
| #?(:clj | ||
| (ns clara.test-performance-optimizations |
There was a problem hiding this comment.
I think it's pretty heavy-weight to fully repeat the whole ns form across the 2 read conditionals.
Ideally we'd be minimizing the need to even specialize most parts of it for clj vs cljs.
For example, :require-macros should be unnecessary in most cases nowadays.
Another good simplification is to be pretty aggressive in the use of ns aliases instead of :refer. That can make it so that the need for any "-macros" cljs declarations are unnecessary too.
This may not be that important for the sake of this review. I just think this will become an annoying pattern to maintain/work with.
There was a problem hiding this comment.
I can take a look at this, either as part of this PR or otherwise. When I initially started creating these test namespaces I wasn't able to find a way to share enough to make it seem worthwhile - IIRC around 80% of the code in the header was in conditionals when I tried to make it a single header. However I could have been missing some options in how to create CLJS ns headers.
Another good simplification is to be pretty aggressive in the use of ns aliases instead of :refer. That can make it so that the need for any "-macros" cljs declarations are unnecessary too.
How does the usage of aliases allow removal of require-macros declarations?
There was a problem hiding this comment.
It's a fairly involved topic.
I think reading/understanding http://blog.fikesfarm.com/posts/2016-03-01-clojurescript-macro-sugar.html is helpful.
We should try to be sure our cljs ns's are written in a way that is supportive of the implicit macro loading behavior described there.
The related Jira (I believe) for that is https://dev.clojure.org/jira/browse/CLJS-948 and was in really old versions of CLJS (earlier than Clara's baseline).
Better :refer handling was added with https://dev.clojure.org/jira/browse/CLJS-1507 as well. This wasn't added to cljs until version 1.9.198 though. Clara is behind this quite a bit.
I do think that Clara is likely trying to support a cljs version that is too old now. That is mentioned in #363 . I don't think there is a large community of people who lag that far back in CLJS. It has fairly rapid version releases. Especially in comparison to CLJ.
| #?(:clj | ||
| (defmacro fail-if-binding-present | ||
| [] | ||
| (when (contains? &env '?unused-binding) |
There was a problem hiding this comment.
Couldn't the macro return a form (just a boolean even) that indicated whether it was a success or failure? The RHS could then use that to conditional do something (eg insert Cold one way vs another) that is testable.
(defmacro unused-binding-present? []
(contains? &env '?unused-binding))There was a problem hiding this comment.
+1 that should work nicely. I'll make the change.
| @@ -73,6 +73,12 @@ | |||
| ;; Function that takes facts and determines what alpha nodes they match. | |||
| get-alphas-fn]) | |||
|
|
|||
| (defn- is-variable? | |||
There was a problem hiding this comment.
Dont we call these "unification variables/bindings" elsewhere? variable seems...vaguer than its actual intent.
EDIT: i just realized you only moved it so nvm
|
👍 |
1 similar comment
|
👍 |
…hen the test fails
…on function
Draft/proposed changes for issue 383.