[m-dev.] for review: add MC++ implementation of library and runtime
Fergus Henderson
fjh at cs.mu.OZ.AU
Sun Dec 10 22:34:36 AEDT 2000
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'.
> +++ mercury_cpp.cpp Mon Dec 4 21:47:42 2000
> @@ -0,0 +1,149 @@
> +// vi: ts=4 sw=4 et tw=0 wm=0
There should be a copyright message at the top of this file.
Also there should be a comment explaining what it is for
and what language it is written in.
> +#using <mscorlib.dll>
> +#using "mercury_il.dll"
> +using namespace System;
It might be a good idea to drop the `using namespace System;' and to
instead use explicit `System::' prefixes in the appropriate places.
> +using namespace mercury;
Likewise here.
> + // 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?
And why do you want it to complain about main being missing?
Isn't it the linker that does linking, not the compiler?
> +
> +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.
> +__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.)
Also method names should be lower case rather than MixedCase.
> + static MR_Char ToChar(MR_Box x)
> + {
> + return (Char) convert_imp::ToInt32(x);
> + }
It would be cleaner to cast to MR_Char rather than
Char here, I think.
> +__gc public class MR_Runtime {
> + public:
> + static void SORRY(MR_String s)
> + {
> + MR_String msg;
> + mercury_exception *ex;
> +
> + msg = String::Concat("Sorry, unimplemented: ", s);
> +
> + ex = new mercury_exception(msg);
> + throw ex;
> + }
I wouldn't bother with declaring the local variable `ex';
the code would be easier to read if you just write
throw new mercury_exception(msg);
> + static void MR_fatal_error(MR_String s)
> + {
> + MR_String msg;
> + mercury_exception *ex;
> +
> + msg = String::Concat("Fatal error: ", s);
> +
> + ex = new mercury_exception(msg);
> + throw ex;
> + }
Likewise here.
> + static int MR_TYPECTOR_REP_ENUM = MR_TYPECTOR_REP_ENUM_val;
> + static int MR_TYPECTOR_REP_ENUM_USEREQ = MR_TYPECTOR_REP_ENUM_USEREQ_val;
> + static int MR_TYPECTOR_REP_DU = MR_TYPECTOR_REP_DU_val;
> + static int MR_TYPECTOR_REP_DU_USEREQ = 3;
> + static int MR_TYPECTOR_REP_NOTAG = 4;
> + static int MR_TYPECTOR_REP_NOTAG_USEREQ = 5;
> + static int MR_TYPECTOR_REP_EQUIV = 6;
> + static int MR_TYPECTOR_REP_EQUIV_VAR = 7;
> + static int MR_TYPECTOR_REP_INT = 8;
> + static int MR_TYPECTOR_REP_CHAR = 9;
> + static int MR_TYPECTOR_REP_FLOAT =10;
> + static int MR_TYPECTOR_REP_STRING =11;
> + static int MR_TYPECTOR_REP_PRED =12;
> + static int MR_TYPECTOR_REP_UNIV =13;
> + static int MR_TYPECTOR_REP_VOID =14;
> + static int MR_TYPECTOR_REP_C_POINTER =15;
> + static int MR_TYPECTOR_REP_TYPEINFO =16;
> + static int MR_TYPECTOR_REP_TYPECLASSINFO =17;
> + static int MR_TYPECTOR_REP_ARRAY =18;
> + static int MR_TYPECTOR_REP_SUCCIP =19;
> + static int MR_TYPECTOR_REP_HP =20;
> + static int MR_TYPECTOR_REP_CURFR =21;
> + static int MR_TYPECTOR_REP_MAXFR =22;
> + static int MR_TYPECTOR_REP_REDOFR =23;
> + static int MR_TYPECTOR_REP_REDOIP =24;
> + static int MR_TYPECTOR_REP_TRAIL_PTR =25;
> + static int MR_TYPECTOR_REP_TICKET =26;
> + static int MR_TYPECTOR_REP_NOTAG_GROUND =27;
> + static int MR_TYPECTOR_REP_NOTAG_GROUND_USEREQ =28;
> + static int MR_TYPECTOR_REP_EQUIV_GROUND =29;
> +
> + static int MR_SECTAG_NONE = 0;
> + static int MR_SECTAG_LOCAL = 1;
> + static int MR_SECTAG_REMOTE = 2;
> +
> +};
There should be a comment here explaining and warning about the code
duplication.
Why aren't these static ints `const'? And why are these members
static ints at all, rather than enumeration constants?
> +__gc public class envptr
> +{
> +public:
> +};
The "public:" access specifier here doesn't add anything;
I think it would be clearer to just delete it.
It would help to have a comment explaining what this class is for.
The name `envptr' may be misleading; `envptr *' suggests a pointer to
a pointer. How about `env_base' or just `environment' instead?
(Make that `EnvBase' or `Environment', since type names should use
MixedCase.)
> +__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.
> +__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.
> +++ mercury_il.il Mon Dec 4 21:37:56 2000
> @@ -0,0 +1,536 @@
There should be some comments at the top of this file;
a copyright notice, and some stuff explaining what this
file is for and what language it is written in.
> +// .module 'generic.dll'
What's that commented-out code for?
Without some comments explaining it, this just
makes things harder to understand.
> +// .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.
> +.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?
> +.method static default int32
> +get_ftn_ptr_mercury__builtin__do_compare__func_0_0() {
> + ldftn void ['mercury'] 'mercury'.'builtin__c_code'::
> + mercury__builtin__do_compare__func_0_0(
> + class System.Object[]&,
> + class System.Object, class System.Object)
> + ret
> +}
> +.method static default int32
> +get_ftn_ptr_mercury__builtin__do_unify__func_0_0() {
> + ldftn int32 ['mercury'] 'mercury'.'builtin__c_code'::
> + mercury__builtin__do_unify__func_0_0(
> + class System.Object, class System.Object)
> + ret
> +}
> +
> +
> +
> +.method static default int32
> +get_ftn_ptr_mercury__builtin__do_compare__float_0_0() {
The spacing here is a bit odd; these methods are separated
by between zero and three blank lines, and I can't discern
any rhyme or reason behind it, except that corresponding
unify and compare ones have no blank lines separating them.
> +.method static default int32
> +get_ftn_ptr_mercury__std_util__do_unify__univ_0_0() {
> + ldftn int32 ['mercury'] 'mercury'.'std_util__c_code'::
> + mercury__std_util__do_unify__univ_0_0(
> + class System.Object, class System.Object)
> + ret
> +}
> +
> +}
It would help to have a comment on the final closing brace,
e.g.
} // class temphack
> +.class public boxedint {
> +.field public int32 val
> +.method default void .ctor(int32)
> +{
> + ldarg.0
> + call instance void System.Object::.ctor()
> + ldarg.0
> + ldarg.1
> + stfld int32 boxedint::val
> + ret
> +}
> +
> +}
I suggest a blank line after the `.class ... {'
and a comment after the final `}'.
Alternatively, you could indent all the .field and .method definitions
that are nested inside a .class by one tab.
Why is this implemented in IL rather than MC++ (or C#, for that matter)?
E.g. doesn't
class boxedint {
public:
int val;
boxedint(int v) : val(v) {}
};
in MC++ have the same effect?
> +.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?
Please document it.
> +.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?
> +.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?
> +.class public generic {
Please document the purpose of this class
and explain why it is implemented in IL.
> +.method static default int32 generic_call2(class System.Object,
> + class System.Object, class System.Object)
> +{
> + ldarg.1
> + ldarg.2
> + ldarg.0
> + call int32 convert_imp::ToInt32(class System.Object)
> + calli int32 (class System.Object, class System.Object)
> + ret
> +}
Please document this code.
Every `ldarg' in this file really ought to have a comment
saying the name of the argument that it is loading.
For every function, there should be documentation of what
the function does, and the names and meaning of the function
parameters.
> Index: scripts/Mmake.rules
...
> +$(os_subdir)%.dll : %.cpp
> + rm -f $(os_subdir)$*.dll
> + $(MSCL) -com+$(MSCL_NOASM) -I$(MERC_C_INCL_DIR) \
> + -I$(MERC_DLL_DIR) $(ALL_MSCLFLAGS) $< -link -noentry \
> + -dll $(MSCL_LIBS) -out:$@
That rule should be conditional on grade `il'/`ilc' (or `il*', etc.)
to avoid causing problems for people who have `.cpp' files in the
same directory as their Mercury files but who aren't using the IL grades.
> +$(os_subdir)%.dll : %.il
> + rm -f $(os_subdir)$*.dll
> + $(MSILASM) $(ALL_MSILASMFLAGS) /dll /quiet /OUT=$@ $<
Likewise that one should be conditional on the grade,
otherwise `mmake' may try to compile via IL to produce DLLs
even when not in an IL grade.
> +#-----------------------------------------------------------------------------#
> +#
> +# Rules for compiling IL files in the user's source directory.
> +#
> +
> +ifneq ("$(ils_subdir)","")
> +
> +.il.dll:
> + $(MSILASM) $(ALL_MSILASMFLAGS) /dll /OUT=$@ $<
Likewise that one should be conditional on the grade.
I think it would help to have a comment explaining the `ifneq'.
You should use /quiet here, to match the earlier patter rule.
> +.il.exe:
> + $(MSILASM) $(ALL_MSILASMFLAGS) /OUT=$@ $<
> + cp default.cfg $*.cfg
>
> +.cpp.dll:
> + $(MSCL) -com+($MSCL_NOASM) -I$(MERCURY_LIBRARY_PATH) $< -link -noentry -dll $(MSCL_LIBS) -out:$@
> +
> +.cpp.exe:
> + $(MSCL) -com+($MSCL_NOASM) -I$(MERCURY_LIBRARY_PATH) $< -link -entry:main $(MSCL_LIBS) -out:$@
These should all be conditional on IL grade.
> +++ scripts/Mmake.vars.in 2000/12/04 12:17:29
> @@ -130,6 +130,26 @@
> CFLAGS =
> EXTRA_CFLAGS =
>
> +MSCL = cl
It would help to have a comment here explaining what `cl' is.
> +ALL_MSCLFLAGS = $(MSCLFLAGS) $(EXTRA_MSCLFLAGS) $(TARGET_MSCLFLAGS) \
> + $(LIB_MSCLFLAGS) $(ALL_CFLAGS)
> +MSCLFLAGS =
> +EXTRA_MSCLFLAGS =
> +LIB_MSCLFLAGS = $(patsubst %,-I %,$(EXTRA_C_INCL_DIRS))
I think it would be good to have a blank line after that one,
before the stuff below:
> +MSCL_NOASM =
> +MSNETSDKDIR = @MSNETSDKDIR@
> +MSCL_LIBS = $(MSNETSDKDIR)/Lib/mscoree.lib \
> + $(MSNETSDKDIR)/Lib/kernel32.lib
> +MERC_C_INCL_DIR = @LIBDIR@/inc
> +MERC_DLL_DIR = @LIBDIR@/inc
s/MSNETSDKDIR/MS_DOTNET_SDK_DIR/g
Some comments here would help.
E.g. I don't know what MSCL_NOASM is for.
Also I think s/MSCL/MS_CL/g would improve readability.
> +MSILASM = @ILASM@
It would help to have a comment here explaining what `cl' is.
Also I suggest s/MSILASM/MS_ILASM/g.
OK, that completes this review. I'd like to see a relative diff
when you've addressed those issues.
Cheers,
Fergus.
--
Fergus Henderson <fjh at cs.mu.oz.au> | "I have always known that the pursuit
| of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh> | -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------
More information about the developers
mailing list