Repository navigation
Gensym / remove the RGF name instead of using f and __args - #147
Conversation
penelopeysm
changed the title
ff and __args
ChrisRackauckas
left a comment
ChrisRackauckas
left a comment
There was a problem hiding this comment.
gensym args would do two things. First, it would make every RGF a unique AST which would break precompilation because then compiling the same function would give you two different hashes. Secondly, it makes it so there is non-freeable memory, so making 1 million RGFs in a session would start to crash the system. Something that does not effect those two properties would be needed.
| f::$RuntimeGeneratedFunctions.RuntimeGeneratedFunction{ | ||
| ::$RuntimeGeneratedFunctions.RuntimeGeneratedFunction{ |
There was a problem hiding this comment.
I think just keep the change to f? The args thing is orthogonal.
There was a problem hiding this comment.
Happy with restricting to f! I'll revert the args bit. I didn't run into issues with the gensym'd args, I know it's pretty hard but is there anyway to test that property?
There was a problem hiding this comment.
If I'm not mistaken, it's still two separate unicode underscores back to back (i.e. it's not _ , IIRC htmlsymbols.xyz/unicode/U+FF3F and one other). If a real human writes code that has the same variable name as that then they probably deserve more problems than that 🤷
There was a problem hiding this comment.
Of course, security by obscurity isn't fully secure, but it's hard to come up with another solution that hits the properties I mentioned above.
There was a problem hiding this comment.
Hmmmmm, they look like ordinary underscores to me:
julia> codeunits("__args")
6-element Base.CodeUnits{UInt8, String}:
0x5f
0x5f
0x61
0x72
0x67
0x73I don't really mind the security by obscurity (I mean after all a gensym is still not fully "secure"), but I suppose if we can reduce the chance of failures from 0.1% to 0.0001% then it would be worth it. Would you be up for changing that identifier to be a more obnoxious Unicode one?
This reverts commit d9cb75d.
|
I'll do the follow up |
|
Thanks Chris! |
Closes #146.
fwas not needed so that could just be removed.For
__argsthe chance of collision is quite small but it's obviously an improvement to gensym it. It was quite annoying to pass a gensym'dargsinto thegenerated_callfunc_body, so I just inlined that function.The newly added tests fail on current master and pass with this PR!