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

Macro Expansion: Special case QuoteNode by Returnsing the unpacked value rather than the QuoteNode itself. #119

Merged
merged 6 commits into from
Sep 14, 2024

Conversation

LilithHafner
Copy link
Owner

@LilithHafner LilithHafner commented Sep 14, 2024

Fixes #99.

This special case assumes that all non-Expr objects in an expression tree evaluate to themselves. This is a false assumption in the case of QuoteNode(::Symbol).

Additionally, this Returns gives the compiler slightly less information by inhibiting constant propagation of literals. Constant propagation of literals is usually bad (misleadingly low runtimes) in this case but this particular fix also results in @b 2 rand and @b 1+1 rand having different properties with the latter more constprop-able. I was not able to find a case where the Returns is helpful.

Returns was added in 4f8a8c8 to avoid compilation time and paired with the test

@testset "no compilation" begin
    res = @b @eval (@b 100 rand seconds=.001)
    @test .001 < 1e-9res.time < .002
    @test res.compile_fraction === 0.0
end

I expect the "no compilation" regression test to regress.

@LilithHafner
Copy link
Owner Author

The regression tests failed as expected. See comment in source code and commit comment of 9ec4ef5 for explanation. The OP is out of date.

@LilithHafner LilithHafner changed the title Remove buggy special case that uses Returns for non-Expr args Macro Expansion: Special case QuoteNode by Returnsing the unpacked value rather than the QuoteNode itself. Sep 14, 2024
@LilithHafner LilithHafner merged commit 99a16dd into main Sep 14, 2024
19 checks passed
@LilithHafner LilithHafner deleted the lh/fix-99 branch September 14, 2024 16:04
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

Successfully merging this pull request may close these issues.

Parsing error on standalone literal symbols
1 participant