Replace gen with fn, gen.{trace,splice} => gen.dynamic.{trace!,splice!} #41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #41 +/- ##
==========================================
- Coverage 66.36% 66.30% -0.06%
==========================================
Files 16 16
Lines 666 653 -13
Branches 22 16 -6
==========================================
- Hits 442 433 -9
- Misses 202 204 +2
+ Partials 22 16 -6
|
sritchie
commented
Sep 11, 2023
src/gen/dynamic.cljc
Outdated
| `(binding [dynamic.trace/*trace* dynamic.trace/no-op] | ||
| ~@body)) | ||
|
|
||
| (defmacro gen |
There was a problem hiding this comment.
(this is just an alias now)
sritchie
commented
Sep 11, 2023
src/gen/dynamic.cljc
Outdated
|
|
||
| (defrecord DynamicDSLFunction [clojure-fn] | ||
| (defmacro trace! [addr [f & xs]] |
There was a problem hiding this comment.
if you really want to go all functions (which I DO!) then we'd change this to a function and change the calls to
(trace! gf x1 x2 ...)and don't try to make it look like a function call. Then trace! is conceptually like apply.
sritchie
force-pushed
the
sritchie/nosplice
branch
2 times, most recently
from
0d3bc33
to
cc5017c
Compare
sritchie
changed the title
sritchie
commented
Sep 29, 2023
| (untraced | ||
| (apply clojure-fn x1 x2 x3 x4 x5 x6 x7 x8 x9 x10 x11 x12 x13 x14 x15 x16 x17 x18 x19 x20 xs))) | ||
| (applyTo [_ xlist] | ||
| (untraced (.applyTo ^clojure.lang.IFn clojure-fn xlist)))] |
There was a problem hiding this comment.
this untraced wrapper was missing on main for clj and cljs.
sritchie
commented
Sep 29, 2023
|
|
||
| (defn trace-form? | ||
| "Returns true if `form` is a trace form." | ||
| [form] | ||
| (and (seq? form) | ||
| (= `gen/trace (first form)))) | ||
| (#{'trace! 'dynamic/trace! 'gen.dynamic/trace!} (first form)))) |
There was a problem hiding this comment.
this is a hack for now, but I want to get things working in SCI before we take on a cleaner solution.
sritchie
commented
Sep 29, 2023
| (or (symbol? gfn) | ||
| (and (seq gfn) | ||
| (= `gen (first gfn))))))))) | ||
| (#{'splice! 'dynamic/splice! 'gen.dynamic/splice!} (first form)))) |
sritchie
force-pushed
the
sritchie/nosplice
branch
from
cc5017c
to
adfeeed
Compare
sritchie
force-pushed
the
sritchie/nosplice
branch
4 times, most recently
from
b4c6186
to
58e8b3a
Compare
zane
approved these changes
Sep 29, 2023
There was a problem hiding this comment.
Suggestions:
- Add a comment above the hack linking it to the issue you made.
- Extract out the code that changes
(trace! …)such that the call isn't wrapped.- Have McCoy or someone from the Gen team review that.
- Change the commit scope from
feattorefactor!if, as it seems, this isn't actually introducing new functionality.
sritchie
force-pushed
the
sritchie/nosplice
branch
from
58e8b3a
to
db72f04
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
gen.{trace,splice}togen.dynamic.{trace!, splice!}to prevent clashes between these functions and thegen.tracenamespacegen.dynamic.trace/without-tracingbecomesgen.dynamic/untracedtraceto(trace! <addr> <gf> & <args>), simplifying the macro implementation and bringing it more inline with GenJAX and Gen.jl (similar change tosplice)trace!andsplice!called outside of agenmacro will now error.untracedcall around the biggest arity ofinvokeforDynamicDSLFunctionThere is a lurking bug that this PR exposed.
trace!andsplice!will currently only work if called astrace!,dynamic/trace!orgen.dynamic/trace!. If you bind the symbol some other way, liked/trace!, tracing will error.We can fix this with some clever macro work for the cljs and SCI environments, but I want to hold off in this PR.