[Mono-dev] patches for RegionInfo support

[Atsushi Eno]

Hi,

Paolo Molaro wrote:
> On 08/16/05 Atsushi Eno wrote:
>>>> +typedef struct {
>>>> +	const stridx_t name;
>>>> +	gint16 region_entry_index;
>>>> +} RegionInfoNameEntry;
>>>> +
>>>> +typedef struct {
>>>> +	gint16 lcid;
>>>> +	gint16 region_entry_index;
>>> It likely makes sense to put region_entry_index inside CultureInfoNameEntry
>>> instead of having a separate lookup array.
>> Mmm, I need clarification here: do you mean, RegionInfoNameEntry
>> could be just replaced by CultureNameInfoEntry ? Then I think yes,
>> I just created it because of its name nature.
>> If you mean having region_entry_index as a member of
>> CultureInfoNameEntry, it is waste of memory, though it is tiny.
>> If you mean unifying region name index array into culture name index
>> array, then the code could be the same, but it is extra cost of
>> lookup, though the cost is tiny here too.
>> Any of the above fixes are of course ok for me.
> 
> I meant in the last structure quoted, RegionLCIDMap. It seems to map
> from a locale id to a region index. If you include the region index into
> the CultureInfoEntry you save the duplicated lcid fields.

Ah, ok that seems much better :-)

>>> This looks wrong: CurrentRegion sounds like a per-thread value and you're 
>>> using
>>> a static var which is per-domain. Also, even if using a simple static var 
>>> is
>>> the right thing, the lock is not needed: the worse that can happe is
>>> that we create two identical objects and one gets collected by the GC.
>> I also guessed so at first, but there seems no CurrentRegion inside
>> a thread unlike CurrentCulture.
> 
> Not sure what you mean: the MS documentation is very confused as usual,
> but it mentions it's per thread.

Oh. Hmm, I meant there is no Thread member that is related to
RegionInfo thus I thought it would be domain specific, and
CurrentRegion is not related to CurrentCulture (changing this
property does not affect on RegionInfo.CurrentRegion value).

For now I'll make change on Thread.cs and RegionInfo.cs so that
every thread will create CurrentRegion on demand as we do the
same for CurrentCulture.

>> Based on the assumption that CurrentRegion is instantiated per-domain,
>> I'm not understanding that one is likely to be collected by the GC
>> (well, one could be collected in another domain, but it does not
>> sound problematic). I think that without the lock CurrentRegion could
>> be different instance in the same domain in a race condition.
> 
> If you hit the race, the worse that can happen is that two equivalent region
> objects are created and they are both stored in the static var: the last
> one will be kept alive and the first one will be collected by the GC.
> So there is no issue that needs locking here, IMHO.

Agreed. I remember that such code that compares CultureInfo instances
could behave unexpectedly (same culture in different instances),
so the same rule would apply to RegionInfo.

Atsushi Eno