-
Notifications
You must be signed in to change notification settings - Fork 35
Replace MacroTools with ExprTools #121
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,64 @@ | ||
import Base: == | ||
|
||
""" | ||
IsEqual(fn) | ||
|
||
Takes a funciton of the form `fn(x)::Bool` | ||
""" | ||
struct IsEqual | ||
fn::Function | ||
end | ||
|
||
(==)(x::IsEqual, y) = x.fn(y) | ||
(==)(y , x::IsEqual) = x == y | ||
|
||
issymbol(x) = x isa Symbol | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove |
||
|
||
""" | ||
get_symbol(expr) | ||
|
||
Return `x` for expressions of form `x::Type` otherwise return nothing | ||
""" | ||
function get_symbol(expr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if expr == Expr(:(::), IsEqual(issymbol), IsEqual(x->true)) | ||
expr.args[1] | ||
else | ||
nothing | ||
end | ||
end | ||
|
||
""" | ||
get_type(x) | ||
|
||
Return `T` if an expresion is of the form `:(x::Type{T})` or `:(::Type{T})` when `T` is a symbol otherwise returns `nothing` | ||
""" | ||
function get_type(expr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here: I propose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With what I proposed above, you could actually subsume it under t, T = splittypeing(:(t::Type{T}))
@assert T !== nothing
_Type, Ts = splitcurly(T)
@assert Ts == (:T,) Although this might get out of hand quickly -- now that I write it, I start to reimplement pattern matching in my head... |
||
if expr == Expr(:(::), Expr(:curly, :Type, IsEqual(issymbol))) | ||
expr.args[1].args[2] | ||
elseif expr == Expr(:(::), IsEqual(issymbol) , Expr(:curly, :Type, IsEqual(issymbol))) | ||
expr.args[2].args[2] | ||
else | ||
nothing | ||
end | ||
end | ||
|
||
""" | ||
getargs_dottilde(x) | ||
|
||
Return the arguments `L` and `R`, if `x` is an expression of the form `L .~ R` or | ||
`(~).(L, R)`, or `nothing` otherwise. | ||
""" | ||
getargs_dottilde(x) = nothing | ||
function getargs_dottilde(expr::Expr) | ||
function getargs_dottilde(expr) | ||
any_arg = IsEqual(x->true) | ||
# Check if the expression is of the form `L .~ R`. | ||
if Meta.isexpr(expr, :call, 3) && expr.args[1] === :.~ | ||
return expr.args[2], expr.args[3] | ||
end | ||
|
||
if expr == Expr(:call, :.~, any_arg, any_arg) | ||
expr.args[2], expr.args[3] | ||
# Check if the expression is of the form `(~).(L, R)`. | ||
if Meta.isexpr(expr, :., 2) && expr.args[1] === :~ && | ||
Meta.isexpr(expr.args[2], :tuple, 2) | ||
return expr.args[2].args[1], expr.args[2].args[2] | ||
elseif expr == Expr(:., :~, Expr(:tuple, any_arg, any_arg)) | ||
expr.args[2].args[1], expr.args[2].args[2] | ||
else | ||
nothing | ||
end | ||
|
||
return | ||
end | ||
|
||
""" | ||
|
@@ -26,12 +67,13 @@ end | |
Return the arguments `L` and `R`, if `x` is an expression of the form `L ~ R`, or `nothing` | ||
otherwise. | ||
""" | ||
getargs_tilde(x) = nothing | ||
function getargs_tilde(expr::Expr) | ||
if Meta.isexpr(expr, :call, 3) && expr.args[1] === :~ | ||
return expr.args[2], expr.args[3] | ||
function getargs_tilde(expr) | ||
any_arg = IsEqual(x->true) | ||
if expr == Expr(:call, :~, any_arg, any_arg) | ||
expr.args[2], expr.args[3] | ||
else | ||
nothing | ||
end | ||
return | ||
end | ||
|
||
############################################ | ||
|
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.
What's the motivation for
IsEqual
? To me it seems it would be much easier (also to read) if we just checkfn(y)
if needed. In functions such asall
orany
you could even just providefn
as first argument directly. In calls such asexpr == Expr(:(::), IsEqual(issymbol), IsEqual(x->true))
you rely on the fact that the internal implementation of(==)(::Expr, ::Expr)
will check==
recursively, which is not guaranteed in general. IMO it would be safer and less prone to surprises if one explicitly checks the parts of an expression one is interested in.Uh oh!
There was an error while loading. Please reload this page.
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 found it to be more readable since the shape of the
Expr
your checking is clearly declared.For example
Vs
But yeah it could be refactored into a functions instead if that's the preferred style.
I played with this idea a bit more and the recursion seems to work ok.
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 don't have a problem with the style per se (although I'm not sure if it will make it easier for people to understand DynamicPPL's source code). The main problem I see is just that the expression matching happens implicitly, by relying on how
==(::Expr, ::Expr)
is implemented which we can't control.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.
Yeah it assumes your familiar with the S-Expr form which is probably the case if you're writing the functions but may not be so if your reading the source or just calling said functions.
I'd think
==(::Expr, ::Expr)
would remain stable since it's quite a fundamental to the language but you're right in that it's not under our control which could be a problem long term if it changes across Julia versions.I'll update the pull to remove the use of
IsEqual
.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.
Maybe it's a good idea if we write a couple of generic helper function s in the style of
splitdef
. Something likef, (arg1, arg2) = splitcall(expr)
,t, T = splittyping(expr)
, etc., in a form that allow tuple unpacking and mark absence withnothing
.