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.

Sign up for GitHub

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

Jump to bottom

[OSX] HybridGlobalization implement locale native functions #84417

Merged
merged 18 commits into from
Apr 16, 2023
Merged

[OSX] HybridGlobalization implement locale native functions #84417

merged 18 commits into from
Apr 16, 2023

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Apr 6, 2023 •
edited
Loading

Implemented following native functions for OSX platforms

  • GlobalizationNative_GetLocaleInfoIntNative,
  • GlobalizationNative_GetLocaleInfoPrimaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleInfoSecondaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleTimeFormatNative

Contributes to #80689
cc @SamMonoRT

@ghost ghost assigned mkhamoyan Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

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

Issue Details

Implemented following native functions for OSX platforms

  • GlobalizationNative_GetLocaleInfoIntNative,
  • GlobalizationNative_GetLocaleInfoPrimaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleInfoSecondaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleTimeFormatNative

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization
Milestone: -

@mkhamoyan mkhamoyan added os-ios Apple iOS os-tvos Apple tvOS labels Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Implemented following native functions for OSX platforms

  • GlobalizationNative_GetLocaleInfoIntNative,
  • GlobalizationNative_GetLocaleInfoPrimaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleInfoSecondaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleTimeFormatNative

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios
Milestone: -

@mkhamoyan mkhamoyan added the os-maccatalyst MacCatalyst OS label Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

Tagging subscribers to 'os-maccatalyst': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Implemented following native functions for OSX platforms

  • GlobalizationNative_GetLocaleInfoIntNative,
  • GlobalizationNative_GetLocaleInfoPrimaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleInfoSecondaryGroupingSizeNative,
  • GlobalizationNative_GetLocaleTimeFormatNative

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios, os-tvos, os-maccatalyst
Milestone: -

@mkhamoyan mkhamoyan requested a review from ilonatommy April 6, 2023 13:46
ilonatommy
ilonatommy reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

Shallow review: looks good. I did not read the logic yet, I will make a 2nd round soon.

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs Show resolved Hide resolved
This was referenced Apr 6, 2023
Open
Closed
ilonatommy
ilonatommy reviewed Apr 7, 2023
View reviewed changes
Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

Great job! I will approve when we finish with resolving comments.

src/libraries/System.Globalization/tests/Hybrid/HybridMode.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Unix.cs Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Show resolved Hide resolved
mdh1418
mdh1418 reviewed Apr 7, 2023
View reviewed changes
src/native/libs/System.Globalization.Native/pal_localeStringData.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_localeStringData.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale_internal.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_localeNumberData.h Outdated Show resolved Hide resolved
src/libraries/Common/src/Interop/Interop.Locale.OSX.cs Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Show resolved Hide resolved
This was referenced Apr 12, 2023
Closed
Closed
@mkhamoyan mkhamoyan requested review from mdh1418 and ilonatommy April 13, 2023 13:15
@mkhamoyan mkhamoyan marked this pull request as ready for review April 13, 2023 13:15
@build-analysis build-analysis bot mentioned this pull request Apr 13, 2023
Closed
mdh1418
mdh1418 reviewed Apr 13, 2023
View reviewed changes
src/native/libs/System.Globalization.Native/pal_locale_internal.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale_internal.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_localeStringData.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_localeNumberData.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.h Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
src/native/libs/System.Globalization.Native/pal_locale.m Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Apr 14, 2023
Closed
mdh1418
mdh1418 approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks :)

@mkhamoyan
Copy link
Contributor Author

Failures are not related.

@mkhamoyan mkhamoyan merged commit eafd523 into main Apr 16, 2023
@mkhamoyan mkhamoyan deleted the hybrid_local_functions branch April 16, 2023 13:54
@mkhamoyan mkhamoyan mentioned this pull request May 2, 2023
Closed
23 tasks
@ghost ghost locked as resolved and limited conversation to collaborators May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdh1418 mdh1418 mdh1418 approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@steveisok steveisok Awaiting requested review from steveisok

@ilonatommy ilonatommy Awaiting requested review from ilonatommy

Assignees

@mkhamoyan mkhamoyan

Labels
area-System.Globalization os-ios Apple iOS os-maccatalyst MacCatalyst OS os-tvos Apple tvOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkhamoyan @mdh1418 @ilonatommy