[m-dev.] for review: add MC++ implementation of library and runtime

[Tyson Dowd]

On 10-Dec-2000, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 04-Dec-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> > 
> > +++ runtime/Mmakefile	2000/12/01 00:14:21
> > @@ -209,6 +209,14 @@
> >  
> >  #-----------------------------------------------------------------------------#
> >  
> > +ifeq ($(GRADE),ilc)
> > + 
> > +MSCL_NOASM=:noAssembly
> 
> It would be a good idea to have some comments here explaining that.
> 
> The test for grade `ilc' is not very robust; in the future
> we are likely to support more than one grade targetting IL,
> e.g. there is already one for --high-level-data (`il').
> At very least you should test for both `il' and `ilc'.
> It would be better to test for `il*', but that's a bit tricky to do.
> You can however test for `*il*' using `findstring'.

Done, and also in library/Mmakefile too.

> > +    // This line (somehow) stops the compiler from
> > +    // linking in the C library (and it will then complain about main being
> > +    // missing)
> > +extern "C" int _fltused=0;
> 
> Why do you want to prevent the compiler from linking in the C library?

1. Because it is native code.
2. Because it causes compile errors because it starts looking for a main
to call from the C runtime.

> And why do you want it to complain about main being missing?

I don't.  I want it to stop complaining.  I've made the comment clearer.

> Isn't it the linker that does linking, not the compiler?

I'm pretty sure they are well and truly integrated in this system.

> > +
> > +namespace mercury {
> > +
> > +
> > +__gc public class mercury_exception : public Exception
> > +{
> > +public:
> > +    mercury_exception()
> > +    { }
> > +
> > +    mercury_exception(MR_String Msg) : Exception(Msg)
> > +    { 
> > +    }
> > +};
> 
> Mercury exceptions should contain a Mercury object.
> At very least there should be some XXX comments here
> explaining how and why this is incorrect/incomplete.
> 
> What's the default constructor [the one with no arguments] for?
> 
> Type names (such as `mercury_exception') should be in MixedCase.

Ok, renamed.

> 
> > +__gc public class mr_convert
> > +{
> > +    public:
> > +    static MR_Box ToObject(MR_Integer x)
> 
> The access specifier "public:" should be unindented (for consistency
> with the code above, and also because it is clearer).
> 
> For consistency with our C coding guidelines, function names like
> `ToObject' should start on a new line.
> (That comment applies throughout this file.)

That's fine.

> Also method names should be lower case rather than MixedCase.

In this case, I'd prefer not to, because they are there to match up with
existing names in the .NET runtime (lots of types support a ToObject
method for instance).  The naming and design of the boxing support
is worth revisiting when boxing works properly.

> > +    static int MR_SECTAG_NONE				= 0;
> > +    static int MR_SECTAG_LOCAL				= 1;
> > +    static int MR_SECTAG_REMOTE				= 2;
> > +
> > +};
> Why aren't these static ints `const'?  And why are these members
> static ints at all, rather than enumeration constants?

const static ints become `literals', which cannot be accessed like fields.
I haven't investigated this lately, but last time I looked there was no
way to access them.  But I think this was just incompleteness.
I think the same problem occurs with enumeration constants.

I've added a comment to this effect, saying we should look into this in
future.

> 
> > +__gc public class envptr
> > +{
> > +public:
> > +};
> 
> The "public:" access specifier here doesn't add anything;
> I think it would be clearer to just delete it.

Default visibility is private, and I've been bitten by this several
times before, and it has taken a significant amount of time to figure
it out (yes, I'm very dense), so I'd prefer to leave the public there to
avoid being bitten again. 

> (Make that `EnvBase' or `Environment', since type names should use
> MixedCase.)

It's now Environment.

> > +__gc public class continuation
> > +{
> > +public:
> > +	envptr *a;
> > +	continuation() {
> > +		this->a = new envptr();
> > +
> > +	}
> > +};
> 
> Hmm, how about a more informative field name than `a'?
> What is that field for?
> And why does it get set in the constructor?
> 
> It would help to have some comments explaining what this class is for.

Dead code, I've deleted it.

> 
> > +__gc public class commit : public Exception
> > +{
> > +public:
> > +};
> 
> The "public:" access specifier here doesn't add anything;
> I think it would be clearer to just delete it.

Same as before.  This is now called Commit.

> > +// .module 'generic.dll'
> 
> What's that commented-out code for?
> Without some comments explaining it, this just
> makes things harder to understand.

Dead code.  

> 
> > +// .assembly extern mscorlib { }
> >
> > +.assembly extern mercury { }
> > +
> > +.assembly extern 'mercury_cpp' { }
> > +
> > +.assembly extern 'mercury.io' { }
> > +
> > +.class public 'mercury.init' {
> > +    .method static default void init_runtime() {
> > +        call void mercury.io::init_state_2_p_0()
> > +    }
> > +}
> 
> Some comments here would probably help.

Ok, fixed and moved inside the mercury.runtime namespace.

> 
> > +.class public temphack {
> > +
> > +.method static default int32 
> > +get_ftn_ptr_mercury__private_builtin__do_compare__typeclass_info_1_0() {
> > +	ldftn void ['mercury'] 'mercury'.'private_builtin__c_code'::
> > +	mercury__private_builtin__do_compare__typeclass_info_1_0(
> > +		class System.Object[], class System.Object[]&,
> > +		class System.Object, class System.Object)
> > +	ret
> > +}
> 
> You definitely need some comments here explaining what is going on
> here.  What is this class for?

Fixed and renamed TempHack.

> 
> > +.class public boxedfloat {
> > +.field public float64 val
> > +.method default void .ctor(float64)
> > +{
> > +	ldarg.0
> > +	call instance void System.Object::.ctor()
> > +	ldarg.0
> > +	ldarg.1
> > +	stfld float64 boxedfloat::val
> > +	ret
> > +}
> > +
> > +}
> 
> Likewise here.
> 
> > +.class public convert_imp {
> 
> What is this class for?  And why is it implement in IL rather than
> some higher-level language?

For doing the implementation of boxing.  It's in IL so I get complete
control of what happens.  This is important because boxing kept breaking
so I wanted to be able to write a working version in this module by
hand (so I could test it by hand).

> Please document it.

Done.

> 
> > +.method static default class System.Object ToObject(int32 ival)
> > +{
> > +	ldarg ival
> > +	newobj instance void boxedint::.ctor(int32)
> > +	ret
> > +}
> > +
> > +.method static default int32 ToInt32(class System.Object obj)
> > +{
> > +	ldarg obj
> > +	ldnull
> > +	beq l1
> > +
> > +	ldarg obj
> > +	isinst class boxedint
> > +	ldfld int32 boxedint::val
> > +	ret
> > +l1:
> > +	ldc.i4 42
> > +	ret
> > +}
> 
> Why does this code test to see if its argument is null and if so,
> return 42?

Dead code, leftovers from when box/unbox was broken.

> 
> > +.method static default float64 ToFloat64(class System.Object obj)
> > +{
> > +	ldarg obj
> > +	ldnull
> > +	beq l1
> > +
> > +	ldarg obj
> > +	isinst class boxedfloat
> > +	ldfld float64 boxedfloat::val
> > +	ret
> > +l1:
> > +	ldc.i4 42
> > +	ret
> > +}
> > +
> > +
> > +}
> 
> Likewise here.  Is returning 42 even type-correct?

Dead code, leftovers from when box/unbox was broken.

[... to be continued ...]

> 
> > +.class public generic {
> 
> Please document the purpose of this class
> and explain why it is implemented in IL.

-- 
       Tyson Dowd           # 
                            #  Surreal humour isn't everyone's cup of fur.
     trd at cs.mu.oz.au        # 
http://www.cs.mu.oz.au/~trd #
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------