Separate RGF code cache from context for variable lookup #22
Conversation
The module for RGF code cache may be different from the context for lookup of global variables used in the function's AST. This change allows these to be separate. Also fix a problem with `show()` mime type and REPL usage — use at the REPL was finding the definition for `show(::IO, ::MIME"text/plain", ::Function)` rather than ours.
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 92.30% 89.36% -2.95%
==========================================
Files 1 1
Lines 39 47 +8
==========================================
+ Hits 36 42 +6
- Misses 3 5 +2
Continue to review full report at Codecov.
|
|
codecov seems confused here, I think those lines are covered. |
| quote | ||
| RuntimeGeneratedFunction(@__MODULE__, $(esc(ex))) | ||
| code = $code | ||
| cache_module = @__MODULE__ |
There was a problem hiding this comment.
Hi @c42f , could I ask if there's a specific reason for setting the cache_module to be @__MODULE__ here, rather than allowing the user to provide it? This unfortunately regresses the RGFPrecompConsumerTest I had in #19 (though it wasn't merged in). I've had to work around this in SciML/ModelingToolkit.jl#769 by manually calling the RuntimeGeneratedFunction constructor instead.
(Edit: just to make it clear, I don't mean for this to sound accusatory or anything, just genuinely wondering if there's some rationale for the cache to always be the calling module rather than user-specified.)
There was a problem hiding this comment.
No offense taken!
The thought was that we were protecting users from accidentally breaking the precompilation cache mechanism (see #21) — originally the function version of RuntimeGeneratedFunction wasn't meant to be used as a constructor; the thought was that the macro would be the official constructor.
However, this doesn't cover all use cases as you noticed. So I've created #26 to allow you to set what you need without messing with the tag struct (which is definitely meant to be internal).
There was a problem hiding this comment.
Thanks a lot mate for your explanation and the work in #26, that'll help simplify the workaround in ModelingToolkit for sure!
The module for RGF code cache may be different from the context for lookup of global variables used in the function's AST. This change allows these to be separate.
Also fix a problem with
show()mime type and REPL usage — use at the REPL was finding the definition forshow(::IO, ::MIME"text/plain", ::Function)rather than ours.Reverts #20 due to the issues discussed in #21. Fixes #21