[Mono-dev] patches for RegionInfo support

Atsushi Eno atsushi at ximian.com
Tue Aug 16 06:47:28 EDT 2005


Hi,

Thanks for comments Paolo. Actually I've already checked in them
with Miguel's suggestion, so am on fixing them.

>> +MonoBoolean
>> +ves_icall_System_Globalization_RegionInfo_construct_internal_region_from_name (MonoRegionInfo *this,
>> +		MonoString *name)
>> +{
>> +	const RegionInfoNameEntry *ne;
>> +	char *n;
>> +	
>> +	MONO_ARCH_SAVE_REGS;
>> +
>> +	n = mono_string_to_utf8 (name);
>> +	ne = bsearch (n, region_name_entries, NUM_REGION_ENTRIES,
>> +			sizeof (RegionInfoNameEntry), region_name_locator);
>> +
>> +	if (ne == NULL) {
>> +                /*g_print ("ne (%s) is null\n", n);*/
>> +        	g_free (n);
>> +		return FALSE;
>> +        }
>> +        g_free (n);
> 
> Indentation is wrong here.

Oops, yes. I'm fixing it, and the original one in CultureInfo stuff.

>> +
>>  #ifdef HAVE_ICU
> 
> Feel free to completely remove the icu stuff with another patch, too;-)
> 
>> Index: culture-info.h
>> ===================================================================
>> --- culture-info.h	(revision 48380)
>> +++ culture-info.h	(working copy)
>> @@ -115,5 +115,28 @@
>>  	gint16 culture_entry_index;
>>  } CultureInfoNameEntry;
>>  
>> +typedef struct {
>> +	gint16 region_id;
>> +	/* gint8 measurement_system; // 0:metric 1:US 2:UK */
>> +	/* gint16 geo_id; */
>> +	const stridx_t iso2name;
>> +	const stridx_t iso3name;
>> +	const stridx_t win3name;
>> +	const stridx_t english_name;
>> +	const stridx_t currency_symbol;
>> +	const stridx_t iso_currency_symbol;
>> +	const stridx_t currency_english_name;
>> +} RegionInfoEntry;
>> +
>> +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.

>> Index: RegionInfo.cs
>> ===================================================================
>> --- RegionInfo.cs	(revision 48291)
>> +++ RegionInfo.cs	(working copy)
>> @@ -22,10 +22,170 @@
>>  // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>  //
>>  using System.Globalization;
>> +using System.Runtime.CompilerServices;
>>  
>>  namespace System.Globalization {
>>  
>> +#if true
>>  	[Serializable]
>> +	public class RegionInfo
>> +	{
>> +		static object forLock = new object ();
>> +		static RegionInfo currentRegion;
>> +
>> +		// This property is not synchronized with CurrentCulture, so
>> +		// we need to use bootstrap CurrentCulture LCID.
>> +		public static RegionInfo CurrentRegion {
>> +			get {
>> +				if (currentRegion == null) {
>> +					CultureInfo ci = CultureInfo.CurrentCulture;
>> +					// If current culture is invariant then region is not available.
>> +					if (ci == null || CultureInfo.BootstrapCultureID == 0x7F)
>> +						return null;
>> +					lock (forLock) {
>> +						// make sure to fill BootstrapCultureID.
>> +						currentRegion = new RegionInfo (CultureInfo.BootstrapCultureID);
>> +					}
>> +				}
>> +				return currentRegion;
>> +			}
>> +		}
> 
> 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.

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.

>> +		bool ConstructInternalRegionFromName (string locale)
>> +		{
>> +			if (!construct_internal_region_from_name (locale))
>> +				return false;
>> +			return true;
>> +		}
>> +
>> +		bool ConstructInternalRegionFromLcid (int lcid)
>> +		{
>> +			if (!construct_internal_region_from_lcid (lcid))
>> +				return false;
>> +			return true;
>> +		}
> 
> These two wrappers look completely useless.

Ah, true. I'll replace them with direct invocations.

Atsushi Eno



More information about the Mono-devel-list mailing list