Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix runtime-extra-platforms failures related to HybridGlobalization #87913

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Jun 22, 2023

Fix failures on runtime-extra-platforms after merging #86895
Return ERROR_COMPARISON_OPTIONS_NOT_FOUND only when options are not found and ERROR_INDEX_NOT_FOUND when substring is not found.

@mkhamoyan mkhamoyan added os-ios Apple iOS os-tvos Apple tvOS os-maccatalyst MacCatalyst OS labels Jun 22, 2023
@mkhamoyan mkhamoyan self-assigned this Jun 22, 2023
@ghost
Copy link

ghost commented Jun 22, 2023

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

Issue Details

Fix failures on runtime-extra-platforms after merging #86895
Return -2 only when options are not found and -1 when substring is not found.

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

os-ios, os-tvos, os-maccatalyst

Milestone: -

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan changed the title Fix runtime-extra-platforms failures Fix runtime-extra-platforms failures related to HybridGlobalization Jun 22, 2023
@mkhamoyan mkhamoyan marked this pull request as ready for review June 22, 2023 10:04
@@ -114,12 +114,15 @@ Range GlobalizationNative_IndexOfNative(const uint16_t* localeName, int32_t lNam
const uint16_t* lpSource, int32_t cwSourceLength, int32_t comparisonOptions, int32_t fromBeginning)
{
assert(cwTargetLength >= 0);
Range result = {-2, 0};
Range result = {-1, 0};
Copy link
Member

Choose a reason for hiding this comment

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

What do these magic numbers of -1, -2, -3, etc. mean?

Copy link
Contributor Author

@mkhamoyan mkhamoyan Jun 22, 2023

Choose a reason for hiding this comment

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

Added ErrorCodes enum to make it clear.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -114,12 +121,15 @@ Range GlobalizationNative_IndexOfNative(const uint16_t* localeName, int32_t lNam
const uint16_t* lpSource, int32_t cwSourceLength, int32_t comparisonOptions, int32_t fromBeginning)
{
assert(cwTargetLength >= 0);
Range result = {-2, 0};
Range result = {ERROR_INDEX_NOT_FOUND, 0};
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be ERROR_COMPARISON_OPTIONS_NOT_FOUND as previously the value was -2?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, that was actually the fix in the first place.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Just please make sure that all related System.Globalization failures in runtime-extra-platforms are resolved before merging.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

Failures are not related.

@mkhamoyan mkhamoyan merged commit fad3d54 into main Jun 22, 2023
@mkhamoyan mkhamoyan deleted the hybrid_tests_failures branch June 22, 2023 18:25
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants