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

[Fergus Henderson]

On 09-Jul-2001, Peter Ross <peter.ross at miscrit.be> wrote:
> 
> Refactor the top level of mlds_to_il so that we only do one pass over
> the MLDS to generate the ILDS.  As a side effect of this change nondet
> code now works again.

Ah, fantastic.

> compiler/ml_code_util.m:
>     Wrapper functions should be static methods not instance methods.
>     Fix ml_gen_label_func_decl_flags to make this true.
...
>  ml_gen_label_func_decl_flags = MLDS_DeclFlags :-
>  	Access = private,  % XXX if we're using nested functions,
>  			   % this should be `local' rather than `private'
> -	PerInstance = per_instance,
> +	PerInstance = one_copy,

Can you explain in more detail why this is necessary?
I don't think this is the right fix.

"label" functions are the nested functions used as continuation labels
for nondeterministic code.  They not "wrapper" functions.

Marking nested functions as "one_copy" is wrong,
and will break the --gcc-nested-functions option.

> 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
> @@ -373,8 +373,30 @@
>  	EnvTypeEntityName = type(EnvClassName, 0),
>  	EnvTypeFlags = env_type_decl_flags,
>  	Fields = list__map(convert_local_to_field, LocalVars),
> +	( Target = il ->

There should be a comment here explaining why you do
things different for IL.

> +		ThisPtr = self(mlds__commit_type),
> +		FieldType = mlds__commit_type,
> +		CtorType = mlds__commit_type,
> +		PtrType = mlds__commit_type,	% XXX

What are all those mlds__commit_types doing there?

> +		FieldName = qual(mlds__append_name(ModuleName,
> +				EnvClassName ++ "_0"), "commit_1"),
> +		Lval = field(no, ThisPtr, named_field(FieldName, CtorType),
> +				FieldType, PtrType),

And here, why is the field named "commit_1"?

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

> @@ -548,7 +570,7 @@
>  	% type declaration.
>  :- func env_type_decl_flags = mlds__decl_flags.
>  env_type_decl_flags = MLDS_DeclFlags :-
> -	Access = private,
> +	Access = public,

You should put a comment here explaining why they need to be public
(e.g. copy the relevant part of the log message).

The environment types should not be public.
Making them public causes implementation details to be included
in the generated C header files for the MLDS->C back-end.

It should be sufficient to use "default" access
(which can be treated as "assembly" access for the .NET back-end).

Currently I think the .NET back-end completely ignores the
MLDS access flags and just makes everything public.

> Index: mlds.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mlds.m,v
> retrieving revision 1.56
> diff -u -r1.56 mlds.m
> --- mlds.m	22 Jun 2001 09:14:35 -0000	1.56
> +++ mlds.m	9 Jul 2001 13:08:59 -0000
> @@ -344,6 +344,9 @@
>  :- func mlds__append_class_qualifier(mlds_module_name, mlds__class_name, arity) =
>  	mlds_module_name.
>  
> +:- func mlds__append_mercury_code(mlds_module_name) = mlds_module_name.
> +:- func mlds__append_name(mlds_module_name, string) = mlds_module_name.

These functions should be documented.

>  :- type mlds__defns == list(mlds__defn).
>  :- type mlds__defn
>  	---> mlds__defn(
> @@ -492,6 +495,7 @@
>  						% inherits these base classes
>  		implements ::	list(mlds__interface_id),
>  						% implements these interfaces
> +		ctors	::	mlds__defns,	% has these constructors
>  		members ::	mlds__defns	% contains these members
>  	).
>  
> @@ -1478,6 +1482,12 @@
>  		name(Package, qualified(Module, ClassQualifier)) :-
>  	string__format("%s_%d", [s(ClassName), i(ClassArity)],
>  		ClassQualifier).
> +
> +mlds__append_mercury_code(name(Package, Module))
> +	= name(Package, qualified(Module, "mercury_code")).
> +
> +mlds__append_name(name(Package, Module), Name)
> +	= name(Package, qualified(Module, Name)).

It would be nicer to define it as

mlds__append_mercury_code(Name) = mlds__append_name(Name, "mercury_code").

> +++ mlds_to_c.m	9 Jul 2001 13:09:00 -0000
> @@ -959,7 +959,14 @@
...
> +		error("mlds_output_class: non empty constructor list")
> +	},

It would be slightly nicer to use unexpected(this_file, "...") here.

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

> +++ mlds_to_gcc.m	9 Jul 2001 13:09:01 -0000
> @@ -1264,7 +1264,12 @@
>  	% not when compiling to C++
>  	%
>  	{ ClassDefn = class_defn(Kind, _Imports, BaseClasses, _Implements,
> -		AllMembers) },
> +		Ctors, AllMembers) },
> +	{ Ctors = [] ->
> +		true
> +	;
> +		sorry(this_file, "constructors")
> +	},

s/sorry/unexpected/

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

> +transform_mlds(mlds(MercuryModuleName, ForeignCode, Imports, Defns))
> +	= mlds(
> +		MercuryModuleName,
> +		ForeignCode,
> +		Imports,
> +		[mercury_code_class(list__map(rename_defn, Members)) | Others]
> +	) :-

This is ugly.  The layout is inconsistent, and you're trying to do too much
in one go.

The following would be better:

transform_mlds(MLDS0, MLDS) :-
	MLDS0 = mlds(MercuryModuleName, ForeignCode, Imports, Defns0),
	MLDS = mlds(MercuryModuleName, ForeignCode, Imports, Defns),
	...
	Defns = [mercury_code_class(list__map(rename_defn, Members)) | Others].

or possibly

transform_mlds(mlds(MercuryModuleName, ForeignCode, Imports, Defns0),
		mlds(MercuryModuleName, ForeignCode, Imports, Defns)) :-
	...
	Defns = [mercury_code_class(list__map(rename_defn, Members)) | Others],

but I prefer the former.

Even better would be to add some field names to the mlds/4 constructor,
and then you could write it as

transform_mlds(MLDS0, MLDS) :-
	Defns = MLDS0 ^ mlds_defns,
	Members = MLDS0 ^ mlds_members,
	...
	Defns = [mercury_code_class(list__map(rename_defn, Members)) | Others],
	MLDS = MLDS0 ^ mlds_defns := Defns.

> +	list__filter((pred(D::in) is semidet :-
> +			( D = mlds__defn(_, _, _, mlds__function(_, _, _))
> +				% XXX we need to place all the RTTI
> +				% datastructures inside this class, so
> +				% they are generated as fields.
> +				% Maybe what we should do is make all
> +				% the RTTI data structures nested
> +				% classes.  I think that would work
> +				% better.
> +			; D = mlds__defn(_, _, _, mlds__data(_, _))

I don't understand this comment.
What do you mean by "this class"?

Putting the comment in the middle of the code like that makes it
harder to read.

> +:- func mercury_code_class(mlds__defns) = mlds__defn.
> +
> +mercury_code_class(Members)
> +	= mlds__defn(
> +		export("mercury_code"),
> +		mlds__make_context(term__context_init),
> +		init_decl_flags(public, per_instance, non_virtual,
> +				final, const, concrete),
> +		mlds__class(
> +			mlds__class_defn(mlds__package, [], [], [], [], Members)
> +		)
> +	).

Please abstract the call to init_decl_flags out into a separate function.

Why don't you use ml_gen_type_decl_flags (from ml_type_gen.m)?

The constness should be "modifiable" rather than "const"
(The constness field doesn't apply to types, and "modifiable"
is the default here.)

> +:- func rename_defn(mlds__defn) = mlds__defn.
> +
> +rename_defn(defn(Name, Context, Flags, Entity0))
> +	= defn(Name, Context, Flags, Entity) :-

That function is misleadingly named.
It doesn't rename the definition, it renames stuff inside the definition.

I suggest s/rename_defn/rename_in_defn/
and likewise elsewhere.

> +	( Entity0 = data(Type, Initializer),
> +		Entity = data(Type, rename_initializer(Initializer))
> +	; Entity0 = function(MaybePredProcId, Params, MaybeStmt0),
> +		( MaybeStmt0 = yes(Stmt),
> +			MaybeStmt = yes(rename_statement(Stmt))
> +		; MaybeStmt0 = no,
> +			MaybeStmt = no
>  		),
> +		Entity = function(MaybePredProcId, Params, MaybeStmt)
> +	; Entity0 = class(_),
> +		sorry(this_file, "renaming nested classes")
> +	).

I think the "sorry" here will break --high-level-data.

> +rename_atomic(new_object(L, T, Type, MaybeSize, C, Args, Types))
> +	= new_object(rename_lval(L), T, Type, MaybeSize,
> +			C, list__map(rename_rval, Args), Types).

Please s/T/Tag/g
and s/C/Context/g

> +:- func rename_code_addr(mlds__code_addr) = mlds__code_addr.
> +
> +rename_code_addr(proc(Label, Signature))
> +	= proc(rename_label(Label), Signature).
> +rename_code_addr(internal(Label, Seq, Signature))
> +	= internal(rename_label(Label), Seq, Signature).

s/rename_label/rename_in_proc_label/

> +:- func rename_data_addr(data_addr) = data_addr.
> +
> +rename_data_addr(data_addr(ModuleName, Name))
> +	= data_addr(append_mercury_code(ModuleName), Name).

It would help to have a comment here...

> +:- func rename_label(mlds__qualified_proc_label) = mlds__qualified_proc_label.
> +
> +rename_label(qual(Module, Name)) = qual(append_mercury_code(Module), Name).

... and here ...

> +:- func rename_var(mlds__var, mlds__type) = mlds__var.
> +
> +rename_var(qual(ModuleName, Name), _Type)
> +	= qual(append_mercury_code(ModuleName), Name).

... and here, saying something along the lines of
This is the interesting bit: this is where we actually do the renaming.
The other parts just traverse the data structures.

In fact it would help to put those three routines at the end,
after all the traversal code.

(Note that these routines should not be renamed "rename_in_*".)

> +:- func mlds_defn_to_ilasm_decl(il_info, mlds__defn) = ilasm__decl.
> +
> +mlds_defn_to_ilasm_decl(_, defn(_Name, _Context, _Flags, data(_Type, _Init)))
> +	= _ :- sorry(this_file, "top level data definition!").

Please add a comment here:

	% IL supports top-level (i.e. "global") function definitions and
	% data definitions, but they're not part of the CLS.
	% Since they are not part of the CLS, we don't generate them,
	% and so there's no need to handle them here.

(XXX I'm not 100% sure about global data... is that correct, Tyson?)

> +mlds_defn_to_ilasm_decl(
...
> +		% XXX Maybe it would be better to just check to see
> +		% whether or not there are any init instructions then
> +		% explicitly checking for the name mercury_code.
> +	( EntityName = "mercury_code" ->

I think definitely, not maybe.

> +class_name(Module, Name) = structured_name(Assembly, ClassName ++ [Name]) :-
> +	ClassName = sym_name_to_list(mlds_module_name_to_sym_name(Module)),
> +	( ClassName = ["mercury" | _] ->
> +		Assembly = "mercury"

There should be a comment here explaining why we treat "mercury"
specially here.

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

> +:- func decl_flags_to_methattrs(mlds__decl_flags) = list(ilasm__methattr).
> +
> +decl_flags_to_methattrs(Flags)
> +	= list__condense([Access, PerInstance, Virtuality,
> +			Finality, Abstractness]) :-
> +	AccessFlag = access(Flags),
> +	( AccessFlag = public,
> +		Access = [public]
> +	; AccessFlag = protected,
> +			% XXX is this correct?
> +		Access = [family]

Yes, that's correct.
Just document this in mlds.m.

> +	; AccessFlag = default,
> +		error("decl_flags_to_methattrs: default access flag")

Use "assembly".

[... to be continued ...]

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------