Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify RuntimeGeneratedFunction type printing, but don't lie about the type :-) #16

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

c42f
Copy link
Contributor

@c42f c42f commented Oct 24, 2020

Alas this is not nearly as pretty as #15, but overloading show(::IO, ::Type{SomeType}) has been known to cause subtle problems in the past. And suspected of causing them quite recently, though I'm not sure it was conclusively proven.

So instead, here I've tried to shorten the RuntimeGeneratedFunction type:

  • Use SHA1 and use UInt32's to encode the 20 bits. sha1 has been good enough for git, and 64 bytes for sha512 is an awful lot.
  • Reorder the RuntimeGeneratedFunction type parameters to make them easier to read.
  • Shorten the module tag name

For example:

julia> typeof(@RuntimeGeneratedFunction(:((x,)->x+1)))
RuntimeGeneratedFunctions.RuntimeGeneratedFunction{(:x,),var"#_RGF_ModTag",(0x191f585f, 0xfaf6f6de, 0xca63d6a8, 0x0d1317a1, 0x08ebef57)}

Which is hardly pretty, but not as bad as before.

This reverts commit 9b9426e.

Even though this printing is lovely to the eye, overloading
show(::IO, ::Type{SomeType}) has been known to cause subtle problems in
the past and suspected of causing them quite recently.
- Use SHA1 and use UInt32's to encode the 20 bits. sha1 has been good
  enough for git, and 64 bytes for sha512 is an awful lot.
- Reorder the RuntimeGeneratedFunction type parameters to make them
  easier to read.
- Shorten the module tag name
@c42f c42f changed the title Simplify RuntimeGeneratedFunction type printing, but don't like about it :-) Simplify RuntimeGeneratedFunction type printing, but don't lie about the type :-) Oct 24, 2020
@c42f c42f mentioned this pull request Oct 24, 2020
@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #16 into master will increase coverage by 8.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   81.81%   90.00%   +8.18%     
==========================================
  Files           1        1              
  Lines          44       40       -4     
==========================================
  Hits           36       36              
+ Misses          8        4       -4     
Impacted Files Coverage Δ
src/RuntimeGeneratedFunctions.jl 90.00% <ø> (+8.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a3f734...173ac50. Read the comment docs.

@ChrisRackauckas
Copy link
Member

I'll let @YingboMa take this.

Seems like a good place to put it in the type hierarchy.
@@ -10,26 +10,17 @@ export @RuntimeGeneratedFunction

This type should be constructed via the macro @RuntimeGeneratedFunction.
"""
struct RuntimeGeneratedFunction{moduletag,id,argnames}
struct RuntimeGeneratedFunction{argnames,moduletag,id} <: Function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha yes, good idea.

@c42f
Copy link
Contributor Author

c42f commented Oct 24, 2020

I also just added a tweak here to make RuntimeGeneratedFunction <: Function as that seems like the right place to add it to the type hierarchy.

@YingboMa some of the history of problems supposedly caused by overloading these kind of show methods can be read about at PainterQubits/Unitful.jl#321 JuliaGeometry/GeometryBasics.jl#67.

To be quite clear, I'm just following those other PRs out of caution in not wanting random segfaults. I don't have a proper explanation for why this doesn't or shouldn't work well :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants