Globalization merge by tarekgh · Pull Request #9835 · dotnet/coreclr

tarekgh · 2017-02-27T23:04:00Z

This change is merging the globalization code used for Linux and Windows. here Is come notes about this merge:

Mainly picked most of the code from the corefx folder as it is well formatted and cleaned up before and then tweaked the implementation to work on Windows
Tries to reduce the dependencies from Globalization code on the VM. this will give flexibility to work on the code without worrying the VM
Provided the missing implementation as I removed many internal calls to the VM. This also help in having .Net Native have complete implementation for .net standard 2.0
We may need to have some small tweaks when merging this globalization code to corert
We'l need to do some clean up for the code which not used anymore in the VM

tarekgh · 2017-02-27T23:05:45Z

@jkotas @ellismg could you please help taking a look?

cc @danmosemsft

jkotas · 2017-02-27T23:12:24Z

+    <MscorlibSources Include="$(BclSourcesRoot)\System\Globalization\JapaneseLunisolarCalendar.cs" />
+    <MscorlibSources Include="$(BclSourcesRoot)\System\Globalization\JulianCalendar.cs" />
    <MscorlibSources Include="$(BclSourcesRoot)\System\Globalization\KoreanCalendar.cs" />
+    <MscorlibSources Include="$(BclSourcesRoot)\System\Globalization\KoreanLunisolarCalendar.cs" />


Shouldn't we be rather keeping the version of System\Globalization* files from under corefx? The copy under corefx is better formatted and factored.

that is what I have done. I copied the files from corefx to BclSourceRoot\S\G and deleted all files in corefx\S\G folder. but the code we have now is what we had in corefx folder.

I am just thinking later we should get rid of the whole corefx folder

I see a bunch of diffs between current corefx\System\Globalization and what's in Globalization with your changes. There are cosmetic changes, some things got converted back to having m_ prefixes, ... .

this may be because we had the formatting changes and I had to merge with it. I'll check renamed m_*

jkotas · 2017-02-27T23:30:11Z

 using System.Collections.Generic;

+#if INSIDE_CLR
+using Kernel32 = Interop.Kernel32;


You do not need these. The CoreRT copy should be switched to kernel32.dll, etc. as well. dotnet/corert#2241

ok I'll clean these then

jkotas · 2017-02-27T23:31:22Z

        {
            EnumCalendarsData data = new EnumCalendarsData();
            data.userOverride = 0;
+#if INSIDE_CLR


We should just use #if CORECLR for these. INSIDE_CLR is hard to decipher.

I didn't introduce INSIDE_CLR, it was there and I just used it. I can use CORECLR but we need to check if this not compiled for corert/PN

I know you have not introduced INSIDE_CLR. We have several different ifdefs that are used for this purpose because of they were introduced by different people at different times. I am suggesting that #if CORECLR is the one we should standardize on because of it is the most logical one.

Opened issue https://github.com/dotnet/coreclr/issues/9839

I checked corert and not using CORECLR as expected. so I'll change my ifdef's to CORECLR.

jkotas · 2017-02-27T23:32:25Z

cc @alexperovich

tarekgh · 2017-02-27T23:59:53Z

@mellinoe did you see such failure in Windows_NT x86 Checked Build and Test (Jit - CoreFx) before? https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x86_checked_windows_nt_corefx_baseline_prtest/18/

looks unrelated to my changes.

Unhandled Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

danmoseley · 2017-02-28T00:30:11Z

/cc @krwq you won't have time to review this but as a Glob co owner you should be aware of it ..

This change is merging the globalization code used for Linux and Windows. here Is come notes about this merge: - Mainly picked most of the code from the corefx folder as it is well formatted and cleaned up before and then tweaked the implementation to work on Windows - Tries to reduce the dependencies from Globalization code on the VM. this will give flexibility to work on the code without worrying the VM - Provided the missing implementation as I removed many internal calls to the VM. This also help in having .Net Native have complete implementation for .net standard 2.0 - We may need to have some small tweaks when merging this globalization code to corert - We'll need to do some clean up for the code which not used anymore in the VM

tarekgh · 2017-02-28T18:49:47Z

@dotnet-bot test this please

tarekgh · 2017-02-28T18:57:54Z

@mmitche do you know why the CI is not running on this PR?

mmitche · 2017-02-28T18:59:31Z

@tarekgh GitHub is having issues with their webhooks. Please see the DDIT outage mail just sent a few minutes ago (and let me know if you didn't get it). They do appear to be coming back online and undelivered hooks are being delivered in some cases.

mellinoe · 2017-02-28T19:34:46Z

@tarekgh We have seen some intermittent issues in the Vector tests, presumably due to the x86 bringup.

@pgavlin were you looking into some of those?

tarekgh · 2017-02-28T19:59:07Z

@jkotas I have fixed all issues according to your comments. please let me know if you have any more comments. thanks.

jkotas · 2017-02-28T21:31:42Z

        {
            EnumCalendarsData data = new EnumCalendarsData();
            data.userOverride = 0;
+#if CORECLR


Other places are using using under #if CORECLR at the top of the file to reduce number of ifdefs. Do the same here?

(And other places with the same pattern.)

jkotas · 2017-02-28T21:45:27Z

+using System.Diagnostics.Contracts;
+using System.Runtime.Serialization;
+using System.Threading;
+#if FEATURE_APPX && !FEATURE_COREFX_GLOBALIZATION


What does FEATURE_COREFX_GLOBALIZATION mean now that you have gotten rid of it? Should this ifdef be deleted?

FEATURE_COREFX_GLOBALIZATION is mainly reflecting Linux. this is how is defined awhile ago. I agree it is not needed here. I'll remove it

jkotas · 2017-02-28T21:46:12Z

+using System.Diagnostics.Contracts;
+using System.Runtime.Serialization;
+using System.Threading;
+#if FEATURE_APPX && !FEATURE_COREFX_GLOBALIZATION


The WinRT specific stuff (FEATURE_APPX) should be in CultureInfo.Windows.cs

Multiple places - there should be no FEATURE_APPX ifdef in this file

jkotas · 2017-02-28T21:51:34Z

@@ -61,11 +61,11 @@ sealed public partial class NumberFormatInfo : ICloneable, IFormatProvider
        internal String numberGroupSeparator = ",";
        internal String currencyGroupSeparator = ",";
        internal String currencyDecimalSeparator = ".";
+#if FEATURE_COREFX_GLOBALIZATION
+        internal String currencySymbol = "$"; // TODO: CoreFX #846 Restore to the original value "\x00a4";  // U+00a4 is the symbol for International Monetary Fund.


This looks like a hack that we forgot to cleanup. Shouldn't we rather get rid of it than keep replicating it?

jkotas · 2017-02-28T22:00:48Z

+            byte [] keyData = null;
+            if (source.Length == 0)
+            { 
+                keyData = EmptyArray<Byte>.Value;


EmptyArray<byte>.Value is not going to compile in CoreRT. Could you please use Array.Empty<byte>() instead?

jkotas · 2017-02-28T22:02:04Z

@@ -82,7 +82,7 @@ private unsafe int GetHashCodeOfStringCore(string source, CompareOptions options

            fixed (char* pSource = source)


Does this have the hashcode randomization like the existing CoreCLR Windows version?

CoreCLR doesn't do randomization on the hash when requesting the linguistic hash code. look at the original

https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Globalization/CompareInfo.cs#L1188

it is passing false for the randomization.
randomized hash done on the string but not on CompareInfo.

The randomized string hashing should be unconditionally enabled in .NET Core. It should not matter what you pass in here, it will be overridden by s_NlsHashProvider.GetUseRandomHashing() in the QCall.

You can remove the bool flag for randomized string hashing while you are on it because of it is unused.

tarekgh · 2017-03-01T00:40:31Z

Thanks @jkotas. I think I have addressed all your comments.

pgavlin · 2017-03-01T01:44:08Z

@mellinoe: that failure is not something I've seen before. I'll look into it.

tarekgh · 2017-03-01T01:54:30Z

@dotnet-bot test Windows_NT x86 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline
@dotnet-bot test Windows_NT x64 corefx_baseline

* Merging the Globalization code in coreclr This change is merging the globalization code used for Linux and Windows. here Is come notes about this merge: - Mainly picked most of the code from the corefx folder as it is well formatted and cleaned up before and then tweaked the implementation to work on Windows - Tries to reduce the dependencies from Globalization code on the VM. this will give flexibility to work on the code without worrying the VM - Provided the missing implementation as I removed many internal calls to the VM. This also help in having .Net Native have complete implementation for .net standard 2.0 - We may need to have some small tweaks when merging this globalization code to corert - We'll need to do some clean up for the code which not used anymore in the VM * Fix ifdef's * Fix field name in linux build * Fix field name in linux compilation * Misc cleanup & return randomized hashing * Fix missing method for Linux * one more minor fix * Fix Linux build

ViktorHofer · 2017-06-30T14:51:01Z

@tarekgh it seems like not all changes from this PR are synced into corert. E.g. CultureInfo._name in coreclr is CultureInfo.m_name in corert.

tarekgh · 2017-06-30T15:28:47Z

@ViktorHofer this PR was merging the globalization implementation between Linux and Windows. it was not touching corert. during that time the serialization was not a concern.

* Merging the Globalization code in coreclr This change is merging the globalization code used for Linux and Windows. here Is come notes about this merge: - Mainly picked most of the code from the corefx folder as it is well formatted and cleaned up before and then tweaked the implementation to work on Windows - Tries to reduce the dependencies from Globalization code on the VM. this will give flexibility to work on the code without worrying the VM - Provided the missing implementation as I removed many internal calls to the VM. This also help in having .Net Native have complete implementation for .net standard 2.0 - We may need to have some small tweaks when merging this globalization code to corert - We'll need to do some clean up for the code which not used anymore in the VM * Fix ifdef's * Fix field name in linux build * Fix field name in linux compilation * Misc cleanup & return randomized hashing * Fix missing method for Linux * one more minor fix * Fix Linux build Commit migrated from dotnet/coreclr@a2c684d

dnfclas added the cla-already-signed label Feb 27, 2017

jkotas reviewed Feb 27, 2017

View reviewed changes

tarekgh added 2 commits February 28, 2017 08:56

Fix ifdef's

8f7ef77

tarekgh force-pushed the GlobalizationMerge branch from 3faedba to 8f7ef77 Compare February 28, 2017 18:46

Fix field name in linux build

a962fca

Fix field name in linux compilation

5a0d470

jkotas suggested changes Feb 28, 2017

View reviewed changes

Misc cleanup & return randomized hashing

c3158a0

jkotas approved these changes Mar 1, 2017

View reviewed changes

tarekgh added 3 commits February 28, 2017 16:53

Fix missing method for Linux

5337917

one more minor fix

f425f93

Fix Linux build

7273cef

tarekgh merged commit a2c684d into dotnet:master Mar 1, 2017

pgavlin mentioned this pull request Mar 1, 2017

Guard a use of a possibly-uninitialized BitSet. #9871

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

filipnavara mentioned this pull request Dec 25, 2018

Makes CultureInfo.get_Parent thread safe. #21671

Merged

tarekgh deleted the GlobalizationMerge branch March 25, 2019 15:30

		@@ -82,7 +82,7 @@ private unsafe int GetHashCodeOfStringCore(string source, CompareOptions options

		fixed (char* pSource = source)

Conversation

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

Uh oh!

Uh oh!

tarekgh commented Feb 27, 2017

Uh oh!

jkotas Feb 27, 2017 • 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!

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!

danmoseley Feb 28, 2017 • 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!

jkotas commented Feb 27, 2017

Uh oh!

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

Uh oh!

Uh oh!

danmoseley commented Feb 28, 2017

Uh oh!

tarekgh commented Feb 28, 2017

Uh oh!

tarekgh commented Feb 28, 2017

Uh oh!

mmitche commented Feb 28, 2017

Uh oh!

mellinoe commented Feb 28, 2017

Uh oh!

tarekgh commented Feb 28, 2017

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!

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!

jkotas Feb 28, 2017 • 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!

tarekgh commented Mar 1, 2017

Uh oh!

pgavlin commented Mar 1, 2017

Uh oh!

tarekgh commented Mar 1, 2017

tarekgh commented Feb 27, 2017 •

edited

Loading

jkotas Feb 27, 2017 •

edited

Loading

danmoseley Feb 28, 2017 •

edited

Loading

tarekgh commented Feb 27, 2017 •

edited

Loading

jkotas Feb 28, 2017 •

edited

Loading