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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Apr 22, 2023
Merged

Support two argument at-irrational #46054

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6711093
Support two argument at-irrational
LilithHafner Jul 15, 2022
ced550e
Replace `eval` at macro expand with `let` at runtime
LilithHafner Jul 18, 2022
b1dfb45
Fixup
LilithHafner Jul 18, 2022
53a0045
whitespace
LilithHafner Jul 23, 2022
50af1d9
add tests
Jul 24, 2022
b2fdfc8
Merge branch 'master' into LilithHafner-patch-2
LilithHafner Oct 11, 2022
8719629
change existing usages of @irrational
Oct 11, 2022
7261fb9
Merge branch 'master' into LilithHafner-patch-2
LilithHafner Nov 25, 2022
a557c9b
fix bug
Nov 25, 2022
b8d6c5f
Merge branch 'master' into LilithHafner-patch-2
LilithHafner Feb 11, 2023
26865b2
Merge branch 'master' into LilithHafner-patch-2
LilithHafner Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions base/irrationals.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,21 @@ end
round(x::Irrational, r::RoundingMode) = round(float(x), r)

"""
@irrational sym val def
@irrational(sym, val, def)
@irrational sym [val] def

Define a new `Irrational` value, `sym`, with pre-computed `Float64` value `val`,
and arbitrary-precision definition in terms of `BigFloat`s given by the expression `def`.
Define a new `Irrational` value, `sym`, with arbitrary-precision definition in terms
of `BigFloat`s given by the expression `def`.

Optionally provide a pre-computed `Float64` value `val` which must equal `Float64(def)`.
`val` will be computed automatically if omitted.
"""
macro irrational(sym, val, def)
irrational(sym, val, def)
end
macro irrational(sym, def)
irrational(sym, Float64(eval(def)), def)
Copy link
Member

@simeonschaub simeonschaub Jul 18, 2022 •
edited
Loading

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:

Suggested change
irrational(sym, Float64(eval(def)), def)
irrational(sym, :(Float64($def)), def)

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

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.

end
function irrational(sym, val, def)
esym = esc(sym)
qsym = esc(Expr(:quote, sym))
bigconvert = isa(def,Symbol) ? quote
Expand Down