Removed duplicated Implementation of CultureAwareComparer #27099

Anipik · 2018-02-13T19:36:05Z

Shared Coreclr Implementation PR - dotnet/coreclr#16334
Related to dotnet/corefx#395
Tests PR - #27051

stephentoub · 2018-02-13T20:05:12Z

src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs

-            {
-                throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
-            }
-


I see this in the .csproj:
https://github.com/Anipik/corefx/blob/c9a7d5c3e4fb4a1ef5897cb0c03440044922a986/src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj#L20-L21
and this should fail when built for netfx as the below overload doesn't exist there, but I see that build passed CI, and it looks like that's because we don't build this library for netfx:
https://github.com/Anipik/corefx/blob/c9a7d5c3e4fb4a1ef5897cb0c03440044922a986/src/System.Globalization.Extensions/src/Configurations.props#L5-L7
@tarekgh, is that just cruft that can be cleaned up?

Yes we don't build netfx package for this library anymore. mainly most of the functionality are already exposed from somewhere else.

@Anipik we just need to delete all the cs files there and clean up the csproj files.

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

@tarekgh can you please tell which files need to be removed ?

We need to delete the files

https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs

https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System/StringNormalizationExtensions.netfx.cs

https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/ApiCompatBaseline.netfx.txt

And then clean up the file
https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj

to remove any support for netfx there.

then we need to update the implementation in the file

https://github.com/dotnet/corefx/blob/a3e4e241c7b7c33750258082969287b9cf45d757/src/System.Runtime.Extensions/src/System/Globalization/Extensions.cs

to remove the CultureAwareComparer and modify the extension method there.

And we need to look at the file

https://github.com/dotnet/corefx/blob/103639b6ff5aa6ab6097f70732530e411817f09b/src/Common/src/CoreLib/System/StringComparer.cs

I think we can remove it too and if any code using this file we can update it to use the new exposed API instead.

cc @weshaggard

tarekgh

please modify the code as suggested in the other comment. Thanks

jkotas · 2018-02-15T18:35:45Z

src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Release|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Windows_NT-Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Windows_NT-Release|AnyCPU'" />
-  <ItemGroup Condition="'$(TargetGroup)' == 'netfx'">


You can delete all netfx conditions from this file.

jkotas · 2018-02-15T18:37:07Z

src/System.Runtime.Extensions/src/System/Globalization/Extensions.cs

-        public override int GetHashCode()
-        {
-            return _compareInfo.GetHashCode() ^ ((int)_options & 0x7FFFFFFF);
+            return StringComparer.Create(CultureInfo.GetCultureInfo(compareInfo.Name), options);


It still does not feel right to me to make this method many times slower with this change.

@tarekgh any other things that we can do ?

It still does not feel right to me to make this method many times slower with this change.

I cannot think of any ready better solution rather than returning back the CultureAwareComparer here. the best we can do is to expose StringCompare.Create which takes a compare info. or we move the whole extension class to coelib

move the whole extension class to coelib

That would be my pick.

@Anipik could you please move this extension class to corelib? thanks a lot for your effort here.

@jkotas do you want to keep this PR open till we move the extension class to corelib? or we can proceed with this PR and have another PR to fix that?

Another thing for this PR, could you please clean up the ref folder too corefx\src\System.Globalization.Extensions\ref?

@jkotas should we move to corelib in this PR or in a different PR ?

@Anipik You may start creating the coreclr PR from now.

tarekgh · 2018-02-15T19:50:07Z

<OmitResources Condition="'$(TargetGroup)' != 'netfx'">true</OmitResources>

remove this condition

Refers to: src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj:12 in b01ad32. [](commit_id = b01ad32, deletion_comment = False)

tarekgh · 2018-02-15T19:51:03Z

src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj

-    <Compile Include="System\Globalization\Extensions.cs" />
-    <Compile Include="System\StringNormalizationExtensions.netfx.cs" />
-  </ItemGroup>
  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">


Condition="'$(TargetGroup)' != 'netfx'" [](start = 13, length = 39)

remove this condition

tarekgh · 2018-02-15T19:51:28Z

remove this block

Refers to: src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj:31 in b01ad32. [](commit_id = b01ad32, deletion_comment = False)

jkotas · 2018-02-16T09:07:05Z

src/System.Globalization.Extensions/ref/System.Globalization.Extensions.csproj

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Release|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uap-Release|AnyCPU'" />
  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">


This is not building for netfx. All conditions for netfx can be deleted too.

Anipik · 2018-02-21T21:28:16Z

@dotnet-bot test this please

Anipik · 2018-02-21T23:38:45Z

@tarekgh @jkotas can you take a look at the changes ?

tarekgh · 2018-02-21T23:49:02Z

@Anipik I assume you have tested it with your private changes in coreclr. right?

jkotas · 2018-02-21T23:51:52Z

src/System.Globalization.Extensions/ref/System.Globalization.Extensions.csproj

+  <ItemGroup>
    <ProjectReference Include="..\..\System.Runtime\ref\System.Runtime.csproj" />
    <ProjectReference Include="..\..\System.Runtime.Extensions\ref\System.Runtime.Extensions.csproj" />
    <ProjectReference Include="..\..\System.Globalization\ref\System.Globalization.csproj" />


I think you can delete \..\System.Globalization\ref\System.Globalization.csproj line.

jkotas · 2018-02-21T23:52:47Z

src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj

    <ProjectGuid>{2B96AA10-84C0-4927-8611-8D2474B990E8}</ProjectGuid>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
-    <OmitResources Condition="'$(TargetGroup)' != 'netfx'">true</OmitResources>
+    <OmitResources>true</OmitResources>


Delete this line and also src\System.Globalization.Extensions\src\Resources\Strings.resx

jkotas · 2018-02-22T00:07:43Z

src/System.Globalization.Extensions/src/System.Globalization.Extensions.csproj

    <Reference Include="System.Diagnostics.Tools" />
    <Reference Include="System.Resources.ResourceManager" />
    <Reference Include="System.Runtime" />
    <Reference Include="System.Runtime.Extensions" />


I think you just need System.Runtime and System.Runtime.Extensions here.

Anipik · 2018-02-22T01:42:12Z

@tarekgh I have tested them locally also and new version of coreclr has also been updated in corefx.

Anipik · 2018-02-22T05:00:54Z

@jkotas is it okay to merge it now ?

…efx#27099) Commit migrated from dotnet/corefx@0fbb5f1

Anipik requested review from danmoseley and tarekgh February 13, 2018 19:36

stephentoub reviewed Feb 13, 2018

View reviewed changes

stephentoub approved these changes Feb 13, 2018

View reviewed changes

tarekgh suggested changes Feb 15, 2018

View reviewed changes

jkotas reviewed Feb 15, 2018

View reviewed changes

tarekgh reviewed Feb 15, 2018

View reviewed changes

jkotas reviewed Feb 16, 2018

View reviewed changes

Anipik mentioned this pull request Feb 20, 2018

Moved extensions.cs to shared folder in coreclr dotnet/coreclr#16459

Merged

tarekgh approved these changes Feb 21, 2018

View reviewed changes

jkotas reviewed Feb 21, 2018

View reviewed changes

jkotas reviewed Feb 22, 2018

View reviewed changes

Anipik added 5 commits February 21, 2018 17:25

Removed duplicated Impleementatuin

12f4dac

Cleanup Done

78a429c

Ref folder cleaned

457b33c

Moved to Coreclr

462112c

More Cleaning

e7b68a0

jkotas approved these changes Feb 22, 2018

View reviewed changes

jkotas merged commit 0fbb5f1 into dotnet:master Feb 22, 2018

karelz added this to the 2.1.0 milestone Mar 10, 2018

Anipik deleted the removeStringComparer branch March 24, 2018 01:34

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Removed duplicated Implementation of CultureAwareComparer (dotnet/cor…

45c13fc

…efx#27099) Commit migrated from dotnet/corefx@0fbb5f1

Removed duplicated Implementation of CultureAwareComparer #27099

Removed duplicated Implementation of CultureAwareComparer #27099

Uh oh!

Conversation

Anipik commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Feb 21, 2018

Uh oh!

Anipik commented Feb 21, 2018

Uh oh!

tarekgh commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Feb 22, 2018

Uh oh!

Anipik commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tarekgh Feb 15, 2018 •

edited

Loading

tarekgh Feb 15, 2018 •

edited

Loading