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

[Carlos Alberto Cortez]

Hey Paolo,

Thanks for answering, comments below:

El vie, 12-08-2005 a las 15:49 +0200, Paolo Molaro escribió:
> 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.
> 

Well, I found that the functions at mono-config.c are a little bit
overheaded IHMO. But we could move there, however. I had a problem
before, and that caused me to move my functions inside assembly.c, but
since I don't rememebr the problem,  I will give a try.

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

Good, will take care of it.

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

Thanks for the typo correction ;-)

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

Good.

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

This was based on the fact I wanted to keep it -at least initially- as
private. We can rename it the way you want, however.

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

That's the way they are associated in .Net in both 1.0 and 1.1. In .Net
2.0 they are domain neutral. So, it's a matter of deciding where we
should put them. 

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

I needed it for freeing in mono_domain_unload, but since we are going to
move it inside mono-config and keep it as domain netrual, we could
remove it.


Thanks Paolo,

Carlos.