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

Couple to ArgCheck? #7

Closed
jw3126 opened this issue Feb 10, 2020 · 8 comments
Closed

Couple to ArgCheck? #7

jw3126 opened this issue Feb 10, 2020 · 8 comments

Comments

@jw3126
Copy link
Contributor

jw3126 commented Feb 10, 2020

This package looks really cool. I thought about doing something like this in ArgCheck.jl, but wanted to keep it free from dependencies and could not figure out a good way to do it.

How about building this package on top of ArgCheck.jl? My idea would be that this package exports @skipargcheck and reexports @argcheck and @check from ArgCheck.jl. Then ArgCheck.jl will just place the begin_argcheck/end_argcheck markers and be usable with zero dependencies stand alone. But it can also be used along with OptionalArgChecks.@skipargcheck which removes the marked blocks. What do you think @simeonschaub ?

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 10, 2020

So the benefit would be very nice default error messages from ArgCheck.jl along with your nice skipping mechanism. Plus you only pay for what you need dependency wise.

@simeonschaub
Copy link
Owner

That sounds like a really cool idea! The one thing, I don't think ArgCheck.jl supports is, instead of the check throwing an error, just make it return something else. This could be addressed though by exporting a macro @optional here, which allows skipping arbitrary code. Do you want to make a PR to ArgCheck.jl, then I could take a look and implement the changes here?

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 10, 2020

Yes sounds cool. I will take care of the ArgCheck.jl (not sure if I can do it this week).
About arbitrary return. One vast generalization of the mechanism here would be a pair of macros

@mark :some_label ...
@elide :some_label ...

Where @elide recursively eliminates all marked blocks with the given laben. This is not only useful for skipping argchecks, but also skipping debugging code, profiling code, OS specific code etc.

@simeonschaub
Copy link
Owner

That should be quite easy to add. I'll have a look into that

@jw3126
Copy link
Contributor Author

jw3126 commented Feb 10, 2020

It seems to me, that the compiler sometimes moves the markers, so that they don't work:

code = quote
    $(Expr(:meta, :begin_optional, :argcheck))
    error()
    $(Expr(:meta, :end_optional, :argcheck))
end
@eval f(x) = $code
@code_warntype f("hello") # looks good

Variables
  #self#::Core.Compiler.Const(f, false)
  x::String

Body::Union{}
1$(Expr(:meta, :begin_optional, :argcheck))
│       Main.error()
│       $(Expr(:meta, :end_optional, :argcheck))
└──     Core.Compiler.Const(:(return), false)
@code_warntype optimize=true f("hello") # looks bad

Variables
  #self#::Core.Compiler.Const(f, false)
  x::String

Body::Union{}
1 ─     invoke Main.error()
│       $(Expr(:unreachable))
│       $(Expr(:meta, :begin_optional, :argcheck))
└──     $(Expr(:meta, :end_optional, :argcheck))

@simeonschaub
Copy link
Owner

I don't think this should actually be a problem, since IRTools, and therefore this package, works on untyped IR and I believe these optimizations only occur at a later stage in the compiler pipeline. I might be wrong though, so if you encounter any problems, please open an issue.

@simeonschaub
Copy link
Owner

If you want to know, what skipargs actually sees, have a look at IRTools.@code_ir instead.

@simeonschaub
Copy link
Owner

Closed by #8

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

No branches or pull requests

2 participants