Skip to content

Temporary fix: capture macros.unit to ensure we are using the right MeasureUnit is used with initIsBaseUnit #19

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
merged 1 commit into from
Mar 26, 2024

Conversation

iCharlesHu
Copy link
Contributor

@iCharlesHu iCharlesHu commented Mar 12, 2024

For some reason the unit being passed to unitIsBaseUnit is the same object MeasureUnit but from a different image (the system ICU). We work around this issue by explicitly capture macros.unit in a variable first, which seems to force the compiler to use the same symbol from the package.

This is a temporary fix.

@iCharlesHu iCharlesHu force-pushed the charles/is-base-unit-fix branch from bf6a38f to ddbee90 Compare March 25, 2024 20:09
@itingliu
Copy link

This commit message is no longer true as @jmschonfeld found out. Can we update it?

@iCharlesHu iCharlesHu force-pushed the charles/is-base-unit-fix branch from ddbee90 to 5e56389 Compare March 26, 2024 01:34
Copy link

@itingliu itingliu left a comment

Choose a reason for hiding this comment

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

For more context about this change - I think we now have evidence that this happens if the client app (in our case, XCTest) loads system Foundation, which loads system ICU, and results in two sets of ICU objects at run time.

@iCharlesHu iCharlesHu changed the title Temporary fix: ensure macros.unit is not deallocated inside of unitIsBaseUnit Temporary fix: capture macros.unit to ensure we are using the right MeasureUnit is used with initIsBaseUnit Mar 26, 2024
@iCharlesHu
Copy link
Contributor Author

Switching to swift-testing does seem to workaround this issue (swift-testing does not should not (please don't) link clang module Foundation). I'll remove this workaround once we switch to swift-testing.

@iCharlesHu iCharlesHu merged commit 40b9706 into swiftlang:main Mar 26, 2024
@iCharlesHu iCharlesHu deleted the charles/is-base-unit-fix branch March 26, 2024 16:55
iCharlesHu added a commit to iCharlesHu/swift-foundation-icu that referenced this pull request Jul 23, 2024
As with the case in swiftlang#19, ICU is getting system ICU confused again. Disambiguate by adding underscore to the type name.
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.

2 participants