[m-rev.] for review: refactor mlds_to_il.m

[Peter Ross]

On Tue, Jul 10, 2001 at 02:46:18AM +1000, Fergus Henderson wrote:
> On 09-Jul-2001, Peter Ross <peter.ross at miscrit.be> wrote:
> > Index: ml_elim_nested.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/ml_elim_nested.m,v
> > retrieving revision 1.29
> > diff -u -r1.29 ml_elim_nested.m
> > --- ml_elim_nested.m	27 Jun 2001 13:41:30 -0000	1.29
> > +++ ml_elim_nested.m	9 Jul 2001 13:08:58 -0000

> > +		Rval = new_object(Lval, no, FieldType, no, no, [], []),
> > +
> > +		Stmt = mlds__statement(atomic(Rval), Context),
> > +		Ctor = mlds__function(no, func_params([], []), yes(Stmt)),
> > +		CtorFlags = init_decl_flags(public, per_instance, non_virtual,
> > +				overridable, modifiable, concrete),
> > +		CtorDefn = mlds__defn(export("unused"), Context, CtorFlags,
> > +				Ctor),
> > +		Ctors = [CtorDefn]
> > +	;
> > +		Ctors = []
> > +	),
> 
> Why do you use `export("unused")' for the constructor name?
> 
> This whole section of code is a mystery to me.
> At very least it needs to be much better documented.
> 
The constructor has to be given an entity name, however the name of a
constructor is determined by the backend (ctor on the ILDS backend,
class name on the Java backend).  Maybe I should add an alternative
which to entity_name which is no_name.


> > +++ mlds_to_csharp.m	9 Jul 2001 13:09:00 -0000
> > @@ -90,8 +90,8 @@
> >  generate_csharp_code(MLDS) -->
> >  
> >  	{ MLDS = mlds(ModuleName, ForeignCode, _Imports, Defns) },
> > -	{ ClassName = mlds_module_name_to_class_name(
> > -		mercury_module_name_to_mlds(ModuleName), yes) },
> > +	{ ClassName = class_name(mercury_module_name_to_mlds(ModuleName), 
> > +			"mercury_code") },
> 
> What's the rationale for that change?
> 
> Increasing the number of places where we hard-code "mercury_code"
> doesn't seem like an improvement.
> 
You have to do this here, formerly the yes argument of
mercury_module_name_to_mlds did this for you.  It doesn't any longer so
you need to do it explicitly.

> > +++ mlds_to_il.m	9 Jul 2001 13:09:02 -0000
> > +	% Move all the top level methods and data definitions into the
> > +	% mercury_code class, and then rename all the definitions as
> > +	% necessary to reflect this new hierachy.
> > +:- func transform_mlds(mlds) = mlds.
> 
> What do you mean by "rename all the definitions"?
> What's the difference between moving them and renaming them?
> 
> Probably you mean to say that you fix all the references to
> those methods and data definitions to reflect their new names.
> 
Yes that is what I mean.  Comment updated.

> > +:- func decl_flags_to_classattrs(mlds__decl_flags) = list(ilasm__classattr).
> > +
> > +decl_flags_to_classattrs(Flags)
> > +	= list__condense([Access, Finality, Abstractness]) :-
> > +	AccessFlag = access(Flags),
> > +	( AccessFlag = public,
> > +		Access = [public]
> > +	; AccessFlag = protected,
> > +		Access = []
> > +	; AccessFlag = private,
> > +		Access = []
> > +	; AccessFlag = default,
> > +		error("decl_flags_to_classattrs: default access flag")
> 
> I suggest you treat "default" as "assembly" (and document this in mlds.m).
> 
> (For some reason ilasm.m doesn't include "assembly" as one of the valid
> "classattrs", only as a "methodattr" and a "fieldattr", but I think that
> is a bug -- the docs on MSDN say that the "assembly" attribute is allowed
> on classes.)
> 
Not according to the docs I have.  There is nested assembly but not
assembly.  Section 9.1.1 of Partition II: Metadata.

--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------