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

use if instead of MacroTools #9

Closed
JeffBezanson opened this issue Jul 22, 2018 · 7 comments
Closed

use if instead of MacroTools #9

JeffBezanson opened this issue Jul 22, 2018 · 7 comments

Comments

@JeffBezanson
Copy link

JeffBezanson commented Jul 22, 2018

MacroTools' pattern matching is very slow, mostly due to using exceptions to signal no-match. This causes JuliaLang/julia#28221. Ideally MacroTools can be fixed too, but in the meantime it seems easier to switch this package to use simple tests like isexpr(ex, :call) && ex.args[1] == :+ instead of MacroTools. Of course that can be abstracted into iscall(ex, :+), etc. --- feel free to go nuts with that.

@ChrisRackauckas
Copy link
Member

That was just added by #7 . We can revert the MacroTools changes as suggested. @devmotion ?

@YingboMa
Copy link
Member

Can I give this issue a try?

@ChrisRackauckas
Copy link
Member

Yes

@devmotion
Copy link
Member

Yes seems like we should replace all occurrences of @capture that were added there. In my opinion the changes nevertheless were helpful to simplify the overall logic of MuladdMacro and added the subtraction handling, so I think we shouldn't just revert everything?

YingboMa added a commit that referenced this issue Jul 22, 2018
YingboMa added a commit that referenced this issue Jul 22, 2018
@MikeInnes
Copy link

Yes, all the utility functions like postwalk are fine, it's only calling @capture recursively that exposes this issue. It looks like the patterns are simple enough that explicit tests won't be a serious readability sacrifice.

YingboMa added a commit that referenced this issue Jul 24, 2018
@YingboMa
Copy link
Member

Closed by #11

@MikeInnes
Copy link

FYI there's a fix for this in this PR – FluxML/MacroTools.jl#104

Would be nice to hear if you see an improvement in this case.

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

5 participants