-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Support two argument at-irrational #46054
Conversation
base/irrationals.jl
Outdated
irrational(sym, val, def) | ||
end | ||
macro irrational(sym, def) | ||
irrational(sym, Float64(eval(def)), def) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely don't want to call eval
inside a macro. Why not:
irrational(sym, Float64(eval(def)), def) | |
irrational(sym, :(Float64($def)), def) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this would be better, but still has the problem of potentially allocating a new BigFloat each time Float64(::Irrational)
is called. In this case the macro should generate a let
binding for the value around those method definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't totally understand macro programming, but when I tested @simeonschaub's suggestion I got
ERROR: MethodError: no method matching Float32(::Expr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've figured it out and implemented what @JeffBezanson recommended.
I don't think the @irrational macro is currently tested at all ( |
Should the existing |
Bump |
Bump. Is there anything else this needs? |
Needs tests; I'll add them if folks think this is a good idea.