-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[OSX] HybridGlobalization Implement casing functions #87919
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
|
||
typedef enum | ||
{ | ||
ERROR_SUCCESS = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: ERROR_NONE
or SUCCESS
or NO_ERROR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we use quite often ERROR_SUCCESS
, like https://github.com/dotnet/runtime/blob/main/src/native/libs/System.Native/pal_networking.c#L709
https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/init/pal.cpp#L787 and many other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah in that case keep as is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR_SUCCESS
is a name from Windows SDK: https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- . It is not a great name. We use it when we are on Windows or when we are trying to emulate Windows. It is not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, will update it, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have globalization error codes defined in
UnknownError = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the rest of the comments are resolved, it looks good to me! Thanks
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good if the exception throwing will be solved in #88180.
Implements a chunk of apple native-api based globalization.
Old, icu-based private API:
GlobalizationNative_ChangeCase
GlobalizationNative_ChangeCaseInvariant
New, non-icu private API:
GlobalizationNative_ChangeCaseNative
GlobalizationNative_ChangeCaseInvariantNative
Contributes to #80689
cc @SamMonoRT