Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix the fired asset in CompareInfo.IndexOf #16373

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Fix the fired asset in CompareInfo.IndexOf #16373

merged 1 commit into from
Feb 14, 2018

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Feb 14, 2018 •
edited
Loading

Fixes #16331

This fix the assert fired in the CI runs:

13:40:53   Assertion Failed
13:40:53   
13:40:53   
13:40:53      at System.Globalization.CompareInfo.IndexOfCore(String source, String target, Int32 startIndex, Int32 count, CompareOptions options, Int32* matchLengthPtr)
13:40:54      at System.Globalization.CompareInfo.IndexOf(String source, Char value, Int32 startIndex, Int32 count, CompareOptions options)
13:40:54      at System.String.Contains(Char value, StringComparison comparisonType)
13:40:54      at System.Tests.StringTests.Contains(String s, Char value, StringComparison comparisionType, Boolean expected) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.Runtime/tests/System/StringTests.netcoreapp.cs:line 144
13:40:54      at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
13:40:54      at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, BindingFlags invokeAttr, Object[] parameters, Object[] arguments)
13:40:54      at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass46_1.<<InvokeTestMethodAsync>b__1>d.MoveNext()
13:40:54      at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
...

@tarekgh
Copy link
Member Author

tarekgh commented Feb 14, 2018

CC @krwq @jkotas

@tarekgh
Copy link
Member Author

tarekgh commented Feb 14, 2018

@ahsonkhan this should fix the failure you saw in the CI run.

ahsonkhan
ahsonkhan reviewed Feb 14, 2018
View reviewed changes
src/mscorlib/shared/System/Globalization/CompareInfo.cs
@@ -819,6 +819,11 @@ public unsafe virtual int IndexOf(string source, char value, int startIndex, int
if (count < 0 || startIndex > source.Length - count)
throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count);

if (source.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only overload that we need to add the sourc.Length check to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we have the check in place in all methods calling Last{IndexOfCore} which firing the assert.

Copy link
Member

@ahsonkhan ahsonkhan Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should the check do something like this:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Globalization/CompareInfo.cs#L853-L860

That is, return 0 if value.Length == 0?

Edit: Nevermind, value is a single char.

Copy link
Member Author

Choose a reason for hiding this comment

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

:-)

In reply to: 168056704 [](ancestors = 168056704)

@benaadams
Copy link
Member

@dotnet-bot test Ubuntu x64 Checked corefx_baseline

@benaadams
Copy link
Member

benaadams commented Feb 14, 2018 •
edited
Loading

baseline test that was failing passes with this change 😄

@tarekgh
Copy link
Member Author

tarekgh commented Feb 14, 2018

I am merging this to unblock the CI

@tarekgh tarekgh merged commit 34083fa into dotnet:master Feb 14, 2018
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Feb 14, 2018
@tarekgh @dotnet-bot
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373)
bff2dc5 
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
tarekgh pushed a commit to dotnet/corefx that referenced this pull request Feb 14, 2018
@dotnet-bot @tarekgh
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373) (#2…
823e019 
…7128)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 15, 2018
@tarekgh @dotnet-bot
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373)
86bd9bf 
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Feb 15, 2018
@tarekgh @jkotas
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373)
790ad48 
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
A-And pushed a commit to A-And/corert that referenced this pull request Feb 20, 2018
@tarekgh @jkotas
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373)
5d4f48f 
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
A-And pushed a commit to A-And/corefx that referenced this pull request Feb 21, 2018
@dotnet-bot @A-And
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373) (do…
ff6c4fc 
…tnet#27128)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
kbaladurin pushed a commit to kbaladurin/corert that referenced this pull request Mar 15, 2018
@tarekgh
Fix the fired asset in CompareInfo.IndexOf (dotnet/coreclr#16373)
8eb2003 
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@tarekgh tarekgh deleted the Fix-the-fired-assert-in-CompareInfo.IndexOf branch March 25, 2019 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ahsonkhan ahsonkhan ahsonkhan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI Failure] Assert failed Linux System.Globalization.CompareInfo.IndexOfCore
3 participants
@tarekgh @benaadams @ahsonkhan