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][globalization] Fix IndexOf on empty strings on iOS to return -1 #111898

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

matouskozak
Copy link
Member

@matouskozak matouskozak commented Jan 28, 2025

Fix incorrect return condition and add test case for IndexOf("", <something>) and "".Contains(<something>).

Clean-up of some styling issues and adding comments.


This PR should fix:

and needs to be backported to .NET 9

Copy link
Contributor

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

@matouskozak
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@matouskozak
Copy link
Member Author

/azp run runtime-ioslike

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@matouskozak matouskozak marked this pull request as ready for review January 28, 2025 17:01
@@ -117,6 +117,11 @@ int32_t GlobalizationNative_CompareStringNative(const uint16_t* localeName, int3
}
}

/**
* Removes zero-width and other weightless characters such as U+200B (Zero Width Space),
Copy link
Member

Choose a reason for hiding this comment

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

  • Removes zero-width and other weightless characters such as U+200B (Zero Width Space),

just in case if this need to change later, it will be good to look at the full list of ignorable characters https://www.unicode.org/charts/collation/chart_Ignored.html

@matouskozak
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks reasonable - but I don't understand the code that well.

@matouskozak matouskozak merged commit 4e01649 into dotnet:main Jan 30, 2025
218 of 221 checks passed
@matouskozak
Copy link
Member Author

/backport to release/9.0-staging

Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13057341292

Copy link
Contributor

@matouskozak backporting to "release/9.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix IndexOf on empty strings on iOS to return -1
.git/rebase-apply/patch:80: trailing whitespace.
 * Removes zero-width and other weightless characters such as U+200B (Zero Width Space), 
.git/rebase-apply/patch:81: trailing whitespace.
 * U+200C (Zero Width Non-Joiner), U+200D (Zero Width Joiner), U+FEFF (Zero Width No-Break Space), 
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IndexOf.cs
M	src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs
Auto-merging src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IndexOf.cs
CONFLICT (content): Merge conflict in src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IndexOf.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix IndexOf on empty strings on iOS to return -1
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

grendello added a commit to grendello/runtime that referenced this pull request Jan 30, 2025
* main: (31 commits)
  More native AOT Pri-1 test tree bring up (dotnet#111994)
  Fix BigInteger outerloop test (dotnet#111841)
  JIT: Run 3-opt once across all regions (dotnet#111989)
  JIT: Check for profile consistency throughout JIT backend (dotnet#111684)
  [JIT] Add legacy extended EVEX encoding and EVEX.ND/NF feature to x64 emitter backend (dotnet#108796)
  [iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 (dotnet#111898)
  System.Speech: Use intellisense xml from dotnet-api-docs (dotnet#111983)
  [mono][mini] Disable inlining if we encounter class initialization failure (dotnet#111754)
  [main] Update dependencies from dotnet/roslyn (dotnet#111946)
  Update dependencies from https://github.com/dotnet/arcade build 20250129.2 (dotnet#111996)
  Try changing the ICustomQueryInterface implementation to always return NotHandled instead of Failed to defer back to the ComWrappers impl. (dotnet#111978)
  Combined dependency update (dotnet#111852)
  Replace OPTIMIZE_FOR_SIZE with feature switch (dotnet#111743)
  Fix failed assertion 'FPbased == FPbased2' (dotnet#111787)
  Add remark to `ConditionalSelect` (dotnet#111945)
  JIT: fix try region cloning when try is nested in a handler (dotnet#111975)
  Use IRootFunctions in Tensor.StdDev (dotnet#110641)
  Remove zlib dependencies from Docker containers (dotnet#111939)
  Avoid `Unsafe.As` for `Memory<T>` and `ReadOnlyMemory<T>` conversion (dotnet#111023)
  Cleanup membarrier portability (dotnet#111943)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants