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

Fix macros #83

Merged
merged 1 commit into from
Jun 8, 2024
Merged

Fix macros #83

merged 1 commit into from
Jun 8, 2024

Conversation

charleskawczynski
Copy link
Member

This PR fixes the issues with the macros, introduced in #79.

Using sin.(AT(rand(1000, 1000))) in the test suite:

main
julia> using Revise; include("test/hygiene.jl")
----------- @time
  0.000047 seconds

this branch
julia> using Revise; include("test/hygiene.jl")
----------- @time
  0.046593 seconds (97 CPU allocations: 22.891 MiB, 12.87% gc time) (6 GPU allocations: 45.776 MiB, 0.44% memmgmt time)

So, this appears to fix the issue. I'm not sure what the best way to test this.

Part of the reason is that this PR is technically behavior changing in that the macro now makes an argument-less closure, and times (in the case of @time) the closure.

I think that this is probably fine and, frankly, we could even eliminate the macro and just make the methods, which are far easier to extend/understand (as there is no hygiene to worry about), user-facing. The purely method-based approaches may be easier to test, too, since we could even check @which to ensure that we're calling the extension when we expect.

@charleskawczynski
Copy link
Member Author

Closes #82.

@Sbozzolo
Copy link
Member

Sbozzolo commented Jun 7, 2024

I kind of a agree. Methos are so much easier to use and understand. They are not as flexible, but I think we can use more powerful tools if/when we need more power.

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove the debug comments before merging.

@charleskawczynski charleskawczynski merged commit e62f126 into main Jun 8, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants