[Mono-dev] Re: [Mono-devel-list] [Patch] Publisher Policy support

[Paolo Molaro]

On 08/11/05 Carlos Alberto Cortez wrote:
> Index: assembly.c
> ===================================================================
> --- assembly.c	(revisión: 47896)
> +++ assembly.c	(copia de trabajo)
[...]
> +void
> +assembly_binding_info_free (AssemblyBindingInfo *info)

This function is exported, but it has a namespace-polluting name.

> +{
> +	g_free (info->name);
> +	g_free (info->culture);
> +}
> +
> +static void
> +start_element (GMarkupParseContext *context, 

Please move all the uses of GMarkup inside mono-config.c and make also
sure it uses the functions there to deal with bundled files.

> +	if (!strcmp (element_name, "assemblyIdentity")) {
> +		for (n = 0; attribute_names [n]; n++) {
> +			const gchar *attribute_name = attribute_names [n];
> +			
> +			if (!strcmp (attribute_name, "name"))
> +				info->name = g_strdup (attribute_values [n]);
> +			else if (!strcmp (attribute_name, "publicKeyToken")) {
> +				if (strlen (attribute_values [n]) == MONO_PUBLIC_KEY_TOKEN_LENGTH - 1)
> +					g_strlcpy (info->public_key_token, attribute_values [n], MONO_PUBLIC_KEY_TOKEN_LENGTH);
> +			}
> +			else if (!strcmp (attribute_name, "culture")) {

The else needs to go into the same line as the '}'.

> +				numbers = version = g_strsplit (*(versions + 1), ".", 4);
> +				info->old_version_top.major = *numbers ? atoi (*numbers++) : -1;
> +				info->old_version_top.minor = *numbers ? atoi (*numbers++) : -1;
> +				info->old_version_top.build = *numbers ? atoi (*numbers++) : -1;
> +				info->old_version_top.revision = *numbers ? atoi (*numbers) : 1;

Is it correct to use 1 here and not -1 as in all the other cases?

> +			}
> +			else if (!strcmp (attribute_name, "newVersion")) {

Usuale else misplacement.

> +static int
> +compare_versions (struct version *v, MonoAssemblyName *aname)

struct version doesn't conform to the mono naming conventions.
Use a typedef and something like MonoAssemblyVersion.

> +static MonoAssemblyName*
> +mono_assembly_bind_version (AssemblyBindingInfo *info, MonoAssemblyName *aname, MonoAssemblyName *dest_name)
> +{
> +	memcpy (dest_name, aname, sizeof (MonoAssemblyName));
> +	dest_name->major = info->new_version.major;
> +	dest_name->minor = info->new_version.minor;
> +	dest_name->build = info->new_version.build;
> +	dest_name->revision = info->new_version.revision;
> +	
> +	return dest_name;
> +}
> +
> +/* LOCKING: Assumes that we are already locked */
> +static AssemblyBindingInfo*
> +search_binding_loaded (MonoAssemblyName *aname)
> +{
> +	GSList *tmp;
> +	MonoDomain *domain = mono_domain_get ();
> +
> +	for (tmp = domain->assembly_bindings; tmp; tmp = tmp->next) {
> +		AssemblyBindingInfo *info = tmp->data;
> +		if (assembly_binding_maps_name (info, aname))
> +			return info;
> +	}

Why do you associate assembly bindings to domains? Since they are stored
in the GAC they seem to be valid for all the domains.

> Index: metadata-internals.h
> ===================================================================
> --- metadata-internals.h	(revisión: 47896)
> +++ metadata-internals.h	(copia de trabajo)
> @@ -236,6 +236,25 @@
>  	MonoDynamicTable tables [MONO_TABLE_NUM];
>  };
>  
> +struct version {
> +	int major;
> +	int minor;
> +	int build;
> +	int revision;
> +};
> +
> +/* Contains information about assembly binding */
> +typedef struct _AssemblyBindingInfo {
> +	char *name;
> +	char *culture;
> +	guchar public_key_token [MONO_PUBLIC_KEY_TOKEN_LENGTH];
> +	int major;
> +	int minor;
> +	struct version old_version_bottom;
> +	struct version old_version_top;
> +	struct version new_version;
> +} AssemblyBindingInfo;

Since this stuff sseems to be used only in one file, there is no point
in exposing them in the headers.

Thanks.

lupus

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