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

Moves String.Comparison.cs to shared location #17035

Closed
wants to merge 1 commit into from

Conversation

marek-safar
Copy link

No description provided.

internal static extern unsafe int nativeCompareOrdinalIgnoreCaseWC(String strA, sbyte* strBBytes);

[MethodImplAttribute(MethodImplOptions.InternalCall)]
private static extern int InternalMarvin32HashString(string s);
Copy link
Member

Choose a reason for hiding this comment

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

I am fixing this one in #17029.

Copy link
Author

@marek-safar marek-safar Mar 19, 2018

Choose a reason for hiding this comment

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

I noticed. I think moving whole GetHashCode (+deps) to String.CoreCLR.cs is best here

Copy link
Member

Choose a reason for hiding this comment

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

Why? It is same between CoreCLR and CoreRT.

//This will not work in case-insensitive mode for any character greater than 0x7F.
//We'll throw an ArgumentException.
[MethodImplAttribute(MethodImplOptions.InternalCall)]
internal static extern unsafe int nativeCompareOrdinalIgnoreCaseWC(String strA, sbyte* strBBytes);
Copy link
Member

Choose a reason for hiding this comment

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

Taking care of this one in #17061

@jkotas
Copy link
Member

jkotas commented Mar 20, 2018

There will be more changes necessary to make the shared implementation build and work well under both CoreCLR and CoreRT.

@marek-safar
Copy link
Author

The shared implementation has only managed code and it should work everywhere. CoreRT already has CompareOrdinalHelper but probably missing some of the dependencies. Mono would like to use the shared implementation and CoreRT can switch to it once it's ready

@danmoseley
Copy link
Member

@jkotas what remains here?

@jkotas
Copy link
Member

jkotas commented Mar 27, 2018

It needs to build and work in CoreRT/ProjectN. I have been working through the issues to make it work well. I think we can close this one since it has conflicts. I will take care of submitting PR with the required changes.

@jkotas jkotas closed this Mar 27, 2018
@jkotas
Copy link
Member

jkotas commented Mar 27, 2018

Superseded by #17247

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants