Update RegionInfo data and Fix RegionInfo.CurrentRegion on Windows #33834
Conversation
|
I couldn't add an area label to this PR. Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future. |
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Globalization/tests/System/Globalization/RegionInfoTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Globalization/tests/System/Globalization/RegionInfoTests.cs
Outdated
Show resolved
Hide resolved
|
@jkotas @stephentoub please let me know if you have any more feedback or we ok to merge. the failures in the CI are unrelated and tracked with other linked issues. |
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Globalization/tests/System/Globalization/RegionInfoTests.cs
Outdated
Show resolved
Hide resolved
| int geoIsoIdLength = Interop.Kernel32.GetGeoInfo(geoId, Interop.Kernel32.GEO_ISO2, ref MemoryMarshal.GetReference(geoIso2Letters), geoIso2Letters.Length, Interop.Kernel32.EN_LANG_ID); | ||
| if (geoIsoIdLength != 0) | ||
| { | ||
| geoIsoIdLength -= geoIso2Letters[geoIsoIdLength - 1] == 0 ? 1 : 0; // handle null termination and exclude it. |
There was a problem hiding this comment.
So sometimes it's null terminated and sometimes not? Why does it not return ERROR_INSUFFICIENT_BUFFER when it has no room to null terminate?
There was a problem hiding this comment.
The doc is not ensuring returning null that is why I have to check to know if we got the null or not.
We are requesting the 2-letters country code and I am providing a buffer of length 10 which is far enough to ensure not running into the ERROR_INSUFFICIENT_BUFFER case even with null.
There was a problem hiding this comment.
From what I can tell, this is always returning zero terminated string.
Would it be best to create the string from the pointer and not bother with using the returned size? I would be fine with not using the Span in that case because of there is no buffer math that can go wrong.
char* pGeoIso2Letters = stackalloc char[10];
if (Interop.Kernel32.GetGeoInfo(..., pGeoIso2Letters, ...) != 0)
{
CultureData? cd = GetCultureDataForRegion(new string(pGeoIso2Letters), ...
There was a problem hiding this comment.
I would be more conservative checking null as the doc didn't ensure that and I am not sure what happen if we run on a different version of Windows.
src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated
Show resolved
Hide resolved
| @@ -152,135 +152,262 @@ internal partial class CultureData | |||
| /// </remarks> | |||
| private static Dictionary<string, string> RegionNames => | |||
| s_regionNames ??= | |||
| new Dictionary<string, string>(211 /* prime */) | |||
| new Dictionary<string, string>(257 /* prime */) | |||
There was a problem hiding this comment.
nit, I don't think there is any value in passing in a prime number - the dictionary internally rounds up to the next prime number from its list. it's harmless though of course.
Ideally the C# compiler would know that there's N entries coming in the initializer and pass in N to the constructor.
There was a problem hiding this comment.
I'll keep it as it is harmless and I am not sure what the C# initializer how it behaves there.
There was a problem hiding this comment.
fine, fyi the initializer always uses the parameterless constructor, so it is a valid microoptmization to pass a size here.
ghost
locked as resolved and limited conversation to collaborators
Fixes #32753
Currently when running on Windows, the call of RegionInfo.CurrentRegion will return the region object matching CurrentCulture. Windows has different settings for the default region than the current culture. The good example is mentioned in the scenario in the attached issue which having current culture was set to
English (Europ)and Country was set toNetherlandand when getting CurrentRegion we return159because Europ doesn't have country code (because it is not a country). We suppose to return the region forNetherland.The change here is fixing this issue and updating the region data table too to reflect latest list of regions we have in the world.