Skip to content

Base.summarysize for Memory with Union #57508

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

Merged

Conversation

PatrickHaecker
Copy link
Contributor

@PatrickHaecker PatrickHaecker commented Feb 23, 2025

Fixes the double accounting of the union byte array in Base.summarysize as described in #57506.

If this is the correct fix, can it be backported to 1.11?

Fix #57506

@PatrickHaecker
Copy link
Contributor Author

This is only one possible fix. The alternative would be to not account for the union byte array in sizeof. This depends on the exact definition of sizeof which is discussed in #54007.

It might be that there is a more consistent way to define sizeof. If this is done, the implementation of Base.summarysize might need to be adapted. However, the current result of Base.summarysize is just wrong and should therefore be fixed.

@giordano giordano changed the title Fix #57506: Base.summarysize for Memory with Union Base.summarysize for Memory with Union Feb 23, 2025
@LilithHafner LilithHafner added the bugfix This change fixes an existing bug label Feb 23, 2025
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, @PatrickHaecker! This looks great to me. I'm going to wait a bit before merging to see of @oscardssmith wants to chime in.

@oscardssmith oscardssmith merged commit 7b7ba33 into JuliaLang:master Feb 23, 2025
10 checks passed
@oscardssmith
Copy link
Member

thanks for this!

@nsajko nsajko added backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Feb 24, 2025
KristofferC pushed a commit that referenced this pull request Feb 26, 2025
Fixes the double accounting of the union byte array in
`Base.summarysize` as described in #57506.

If this is the correct fix, can it be backported to 1.11?

Fix #57506

(cherry picked from commit 7b7ba33)
KristofferC pushed a commit that referenced this pull request Mar 11, 2025
Fixes the double accounting of the union byte array in
`Base.summarysize` as described in #57506.

If this is the correct fix, can it be backported to 1.11?

Fix #57506

(cherry picked from commit 7b7ba33)
@KristofferC KristofferC mentioned this pull request Mar 11, 2025
71 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Mar 24, 2025
KristofferC pushed a commit that referenced this pull request Mar 25, 2025
Fixes the double accounting of the union byte array in
`Base.summarysize` as described in #57506.

If this is the correct fix, can it be backported to 1.11?

Fix #57506

(cherry picked from commit 7b7ba33)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base.summarysize incorrect for Memory containing Union
5 participants