[Mono-dev] patches for RegionInfo support

[Paolo Molaro]

On 08/15/05 Atsushi Eno wrote:
> I noticed that currently our RegionInfo (sys.globalization)
> support is hard-coded and pretty insufficient. So I reimplemented
> it like what we do for CultureInfo i.e. create region table from
> CLDR via locale-builder.
> 
> Actually I mostly reused locale-builder so it just requires
> attached patchs. The resulting culture-info-table.h will become
> like 175 KB where it currently is about 140 KB.

The size of the header file doesn't matter, what matters is the
resulting binary size. Anyway, I'm sure it's not a big deal.
There are trivial ways to compress the existing data, too.

> +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.

> +
>  #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.

> 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.

> +		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.

Thanks.
lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better