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

removed @pure's #69

Merged
merged 1 commit into from
Feb 18, 2022
Merged

removed @pure's #69

merged 1 commit into from
Feb 18, 2022

Conversation

caseykneale
Copy link
Contributor

@caseykneale caseykneale commented Feb 16, 2022

Working with @NHDaly we found that the pure annotations didn't change lowered code in most circumstances(Julia 1.6.5), likely due to improvements in the Julia compiler. The @pure annotations however do introduce potential trouble when used outside of their appropriate contexts (JuliaLang/julia#39954 (comment)).

@omus
Copy link
Contributor

omus commented Feb 17, 2022

I'm good for this as long as @NHDaly approves. It may be reasonable to also update the minimum version of Julia required here if we think this could impact older version of Julia negatively. We should probably drop support for Julia versions before Julia 1.6 anyway so that works out.

@NHDaly
Copy link
Member

NHDaly commented Feb 18, 2022

Yeah, i'm good for this. Frankly, i just didn't know what I was doing back when I introduced all those @pures. I think that they're all probably correct/fine as long as you only used FixedDecimal with types from base, like Int64 but they're definitely not fine if you use it with your own custom Integer type.

I had been meaning to go back and delete all these @pure annotations for a long time, once I understood that they're a safety violation. I'll feel much better about us finally tagging a release once these are gone! 😊

Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

Even if this turns out to be a performance regression, I believe it's worth it because we were previously violating safety rules. 👍

We can reinvestigate how to improve the performance if we ever find ourselves needing to come back to this!

@NHDaly NHDaly merged commit 8aa24e8 into JuliaMath:master Feb 18, 2022
@NHDaly
Copy link
Member

NHDaly commented Feb 18, 2022

Oh shoot, sorry @omus: I forgot about your comment to first raise the lower-bound on the Julia version. Lemme do that now, too, in a separate PR.

@NHDaly
Copy link
Member

NHDaly commented Feb 18, 2022

🙄 and then i accidentally committed directly to master. Anyway, i've done that here:
460eb8f

I think we could make a release now, too! I'll go ahead and do that 🎉

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.

3 participants