[m-rev.] for review: refactor mlds_to_il.m
Peter Ross
peter.ross at miscrit.be
Wed Jul 11 00:26:58 AEST 2001
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
--------------------------------------------------------------------------
More information about the reviews
mailing list