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

[iOS][non-icu] HybridGlobalization rafactor condition checks / clean up the code #96328

Closed
4 tasks done
mkhamoyan opened this issue Dec 27, 2023 · 2 comments · Fixed by #96974
Closed
4 tasks done

[iOS][non-icu] HybridGlobalization rafactor condition checks / clean up the code #96328

mkhamoyan opened this issue Dec 27, 2023 · 2 comments · Fixed by #96974
Assignees
Milestone

Comments

@mkhamoyan
Copy link
Contributor

mkhamoyan commented Dec 27, 2023

  • Define APPLE_HYBRID_GLOBALIZATION symbol for the 3 OSes and use it throughout the implementation instead of defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS) conditions in all files under https://github.com/dotnet/runtime/tree/main/src/native/libs/System.Globalization.Native
  • HybridGlobalization is supported only on iOS/tvOS/macCatalyst not OSX. Rename IsHybridGlobalizationOnOSX / IsNotHybridGlobalizationOnOSX to IsHybridGlobalizationOnApplePlatform / IsNotHybridGlobalizationOnApplePlatform https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs#L384.
  • Remove IsIcuGlobalizationAndNotHybrid property
  • Check if possible to include "pal_utilities.h" in pal_placeholders.h instead of defining asserts, look discussion [HybridGlobalization] Include all globalization symbols #96684 (comment)
    we shouldn't mix our configs with pal_config.h.in, so we will keep the way it was implemented.

Contributes to #80689

@ghost
Copy link

ghost commented Dec 27, 2023

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Define APPLE_HYBRID_GLOBALIZATION symbol for the 3 OSes and use it throughout the implementation instead of defined(TARGET_MACCATALYST) || defined(TARGET_IOS) || defined(TARGET_TVOS) conditions in all files under https://github.com/dotnet/runtime/tree/main/src/native/libs/System.Globalization.Native
  • HybridGlobalization is supported only on iOS/tvOS/macCatalyst not OSX. Rename IsHybridGlobalizationOnOSX / IsNotHybridGlobalizationOnOSX to IsHybridGlobalizationOniOS / IsNotHybridGlobalizationOniOS https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs#L384.

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 27, 2023
@jkotas
Copy link
Member

jkotas commented Dec 27, 2023

IsHybridGlobalizationOniOS

This should be IsHybridGlobalizationOnApplePlatform. It returns true on more OSes, not just iOS. The name does not need to encode the fact that the hybrid globalization is not available in macOS build currently.

@mkhamoyan mkhamoyan changed the title [iOS][non-icu] HybridGlobalization rafactor condition checks [iOS][non-icu] HybridGlobalization rafactor condition checks / clean up the code Jan 10, 2024
@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Jan 10, 2024
@steveisok steveisok added this to the 9.0.0 milestone Jan 10, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 15, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 18, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants