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

Fergus Henderson fjh at cs.mu.OZ.AU
Tue Dec 5 02:06:26 AEDT 2000


On 04-Dec-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> First implementation of the standard library in managed C++.

Great!

> configure.in:
> 	Autodetect the .NET SDK, and set MSNETSDKDIR based on it.

I think MS_DOTNET_SDK_DIR would be a better variable name.
Or perhaps just DOTNET_SDK_DIR if you prefer.

> library/*.m:
> 	Add pragma foreign_code for MC++.
> 	Rename pragma c_code as pragma foreign_code("C", ...).
> 	Only a fraction of the predicates are implemented, everything
> 	else simply throws and exception when called.
> 	Implementations of predicates marked with :- external are
> 	provided as pragma foreign_code, but are commented out.

The log message here is a little confusing.
I suggest reordering it and rewording a couple of things.

 	Rename pragma c_code as pragma foreign_code("C", ...).
 	Add pragma foreign_code for MC++.
 	Only a fraction of the predicates are implemented in MC++,
	everything else simply throws an exception when called.
 	Implementations of predicates marked with :- external are
 	provided as pragma foreign_code, but are commented out.

> runtime/mercury_cpp.cpp:
> 	Implementation of the runtime.

Since that file is in Managed C++, not standard C++,
I suggest you name it mercury_mcpp.cpp.

> +++ library/Mmakefile	2000/12/01 03:34:04
> @@ -86,6 +86,8 @@
>  			$(INTERMODULE_OPTS) $(CHECK_TERM_OPTS)
>  MGNUC	=	$(M_ENV) $(SCRIPTS_DIR)/mgnuc
>  MGNUCFLAGS =	$(DLL_CFLAGS)
> +MSCLFLAGS  =	-I$(RUNTIME_DIR) 
> +MSCL_NOASM=:noAssembly

You'd better document what those flags are for.

(The others, e.g. MGNUCFLAGS, are documented in the Mmake chapter
of the Mercury User's Guide.)

> -.PHONY: os cs
> +.PHONY: os cs ils
>  os: $(library.os)
>  cs: $(library.cs)
> +ils: $(library.ils)
> +library.dlls = $(library.mods:%=%.dll)
> +dlls: $(library.dlls)

There should be `.PHONY:' rules for `library.dlls' and `dlls'.

> +ifeq ($(GRADE),ilc)

It would be a good idea to have a comment here explaining what the
`ilc' grade is.

Shouldn't the same rule apply for the `.il' grade too?

> +lib_std: dlls mercury.dll
> +
> +LIBDLLS=$(library.dlls:%=%)
> +RUNTIMEDLLS=../runtime/mercury_cpp.dll ../runtime/mercury_il.dll
> +CPPDLLS=	array__c_code.dll 		\
> +		benchmarking__c_code.dll	\
> +		builtin__c_code.dll		\
> +		char__c_code.dll		\
> +		exception__c_code.dll		\
> +		float__c_code.dll		\
> +		gc__c_code.dll			\
> +		int__c_code.dll			\
> +		io__c_code.dll			\
> +		library__c_code.dll		\
> +		math__c_code.dll		\
> +		private_builtin__c_code.dll	\
> +		sparse_bitset__c_code.dll	\
> +		std_util__c_code.dll		\
> +		store__c_code.dll		\
> +		string__c_code.dll		\
> +		table_builtin__c_code.dll	\
> +		time__c_code.dll
> +ALLDLLS=$(LIBDLLS) $(CPPDLLS) $(RUNTIMEDLLS) 

Please use underscores in the variable names, it makes them much
easier to read.  This applies throughout this file.
For example, s/LIBDLLS/LIB_DLLS/

Also, it would be nicer to avoid the repeated occurrences of __c_code.dll
and instead write

	MODULES_CONTAINING_C_CODE = \
		array \
		benchmarking \
		etc.
	
	CPP_DLLS = $(MODULES_CONTAINING_C_CODE:%=%__c_code.dll)

(or some appropriate descriptive name if MODULES_CONTAINING_C_CODE
is not appropriate).

The line

	LIBDLLS=$(library.dlls:%=%)

should be just

	LIBDLLS=$(library.dlls)

It might in fact be clearer to just use $(library.dlls) whenever
$(LIBDLLS) is used.

> +EMBEDALLDLLS=$(foreach file,$(ALLDLLS),/embed:$(file),$(patsubst %.dll,%,$(file)),Y)

Hmm, I couldn't parse that.  The `$(foreach ...)' command expects
something of the form `$(foreach VAR,LIST,TEXT)', but your foreach
command has four commas in it (not counting the ones nested inside `$(patsubst ...)'),
not two.

I guess you and/or GNU Make are assuming that TEXT can contain commas.
But this isn't documented in the GNU Make manual.

> +
> +mercury.dll: $(ALLDLLS)
> +	al -out:mercury.dll $(ALLDLLS)

If you bother to autoconfigure ILASM, shouldn't you also autoconfigure "al"?

A comment here explaining what `al' is would be helpful.

> Index: library/array.m
> +:- pragma foreign_code("MC++", "
...
> +    static int
> +    mercury__array__do_unify__array_1_0(MR_Word type_info, MR_Box x, MR_Box y)
> +    {
> +            return mercury__array____Unify____array_1_0(
> +                    type_info, 
> +                    dynamic_cast<MR_Word>(x),
> +                    dynamic_cast<MR_Word>(y));
> +    }
> +
> +    static void
> +    mercury__array__do_compare__array_1_0(
> +            MR_Word type_info, MR_Word_Ref result, MR_Box x, MR_Box y)
> +    {
> +            mercury__array____Compare____array_1_0(
> +                    type_info, result, 
> +                    dynamic_cast<MR_Word>(x),
> +                    dynamic_cast<MR_Word>(y));
> +    }
> +").

You should cast to MR_Array rather than MR_Word.

> +:- pragma foreign_code("MC++", 
> +		array__init(Size::in, Item::in, Array::array_uo),
> +		[will_not_call_mercury, thread_safe], "
> +		// XXX still need to do init
> +	Array = (MR_Word) Array::CreateInstance(Item->GetType(), Size);
> +").

The XXX there is bad enough that I think it would be better
to call SORRY() at this point.

I think it would be clearer to spell out `System.Array'.

> +:- pragma foreign_code("MC++",
> +		array__make_empty_array(Array::array_uo),
> +		[will_not_call_mercury, thread_safe], "
> +	Array = (MR_Word) 
> +		Array::CreateInstance((new Object)->GetType(), 0);
> +").

It's quite disturbing that this allocates an array of a different type
than array__init.  I suspect that one of them must be wrong.

It's inefficient to allocate a new object each time just to get the
type of System.Object.  There must be a better way to do that.
For example, it would be better to have

	static System.Object *obj = new System.Object;
	Array = (MR_Word) 
		Array::CreateInstance(obj->GetType(), 0);

if that is allowed.

> +:- pragma foreign_code("MC++",
> +		array__copy(Array0::array_ui, Array::array_uo),
> +		[will_not_call_mercury, thread_safe], "
> +		// XXX need to deep copy it
> +	Array = Array0;
> +
> +").
> +
> +:- pragma foreign_code("MC++",
> +		array__copy(Array0::in, Array::array_uo),
> +		[will_not_call_mercury, thread_safe], "
> +		// XXX need to deep copy it
> +	Array = Array0;
>  ").

Again, I think it would be better to just call SORRY().

> Index: library/benchmarking.m
...
> +:- pragma foreign_code("MC++", report_stats, will_not_call_mercury,
> +"
> +	// Do nothing
> +").
> +
> +:- pragma foreign_code("MC++", report_full_memory_stats, will_not_call_mercury,
> +"
> +	// Do nothing
> +").

There should be some XXX comments there.
I'm sure the dotnet library must provide some access to performance statistics.

> +:- pragma foreign_code("MC++",
> +	get_user_cpu_miliseconds(Time::out), [will_not_call_mercury],
> +"
> +	// This won't return the elapsed time since program start,
> +	// as it begins timing after the first call.
> +	// For computing time differences it should be fine.
> +	// XXX this is documented but not here.  Perhaps it is fixed
> +	// in .NET Frameworks Beta1?
> +	// Time = (int) (1000 * Diagnostics::Counter::GetElapsed());
> +	MR_Runtime::SORRY(""foreign code for this function"");
> +").

The comment here is confusing.
What is documented but not here?
What do you mean by "here"?

>  /*
>  ** To prevent the C compiler from optimizing the benchmark code
>  ** away, we assign the benchmark output to a volatile global variable.
>  */
>  
> -:- pragma c_header_code("
> +:- pragma foreign_decl("C", "
>  	extern volatile MR_Word ML_benchmarking_dummy_word;
>  ").
> -:- pragma c_code("
> +:- pragma foreign_code("C", "
>  	volatile MR_Word ML_benchmarking_dummy_word;
>  ").
>  
>  :- impure pred do_nothing(T::in) is det.
> -:- pragma c_code(do_nothing(X::in), [will_not_call_mercury, thread_safe], "
> +:- pragma foreign_code("C", 
> +	do_nothing(X::in), [will_not_call_mercury, thread_safe], "
> +	ML_benchmarking_dummy_word = (MR_Word) X;
> +").
> +:- pragma foreign_code("MC++", 
> +	do_nothing(X::in), [will_not_call_mercury, thread_safe],
> +"
> +	volatile MR_Word ML_benchmarking_dummy_word;
>  	ML_benchmarking_dummy_word = (MR_Word) X;
>  ").

The comment above says that you assign the output to a volatile global
variable, but here you're assigning it to a volatile local variable.

I think it would be safer to assign it to a volatile static variable.

> Index: library/builtin.m
...
> +:- pragma foreign_code("MC++", compare(Res::uo, X::in, Y::in),
> +	[may_call_mercury], "
> +
> +        MR_TypeInfo             type_info;
> +        MR_TypeCtorInfo         type_ctor_info;
> +        int                     arity;
> +        MR_TypeInfoParams       params;
> +        MR_Word                 *args;
> +
> +        type_info = (MR_TypeInfo) TypeInfo_for_T;
> +        type_ctor_info = dynamic_cast<MR_Word> (type_info->GetValue(0));
> +
> +        if (type_ctor_info == 0) {
> +            type_ctor_info = type_info;
> +        }
> +
> +        if (0) {
> +            // code for higher order...

There should be an XXX there.

> +        } else {
> +            arity = mr_convert::ToInt32(type_ctor_info->GetValue(0));
> +            // params = ???
> +        }

I don't understand the "params = ???" comment there.

> +        switch(arity) {
> +	case 0: 
> +        	generic::generic_res_call3(
> +			type_ctor_info->GetValue(3),
> +			&Res, X, Y);
> +	break;
> +	case 1:
> +                generic::generic_res_call4(
> +			type_ctor_info->GetValue(3), 

The indentation when quoted is odd here -- you are mixing tabs and
spaces.

I think it would be a very good idea to assign
type_ctor_info->GetValue(3), which is used in all
of the cases, to a named local variable.

The numbering of the generic_res_call functions is odd;
why is it called `call3', rather than `call0'?

> +:- pragma foreign_code("MC++", copy(_X::ui, _Y::uo),
> +	[may_call_mercury], "
> +	MR_Runtime::SORRY(""foreign code for this function"");
> +").
> +
> +:- pragma foreign_code("MC++", copy(_X::in, _Y::uo),
> +	[may_call_mercury], "
> +	MR_Runtime::SORRY(""foreign code for this function"");
> +").

I tried to refrain from commenting on calls to SORRY(), but this one
looks fairly simple to implement: isn't there a Clone() function or
something like that in System.Object?

> +:- pragma foreign_code("MC++", unify(X::in, Y::in),
> +	[may_call_mercury], "
> +{
> +        MR_TypeInfo             type_info;
> +        MR_TypeCtorInfo         type_ctor_info;
> +        MR_Box                  tmp;
> +        int                     arity;
> +        MR_TypeInfoParams       params;
> +	
> +        type_info = (MR_TypeInfo) TypeInfo_for_T;
> +
> +        type_ctor_info = dynamic_cast<MR_Word> (type_info->GetValue(0));
> +        if (type_ctor_info == 0) {
> +            type_ctor_info = type_info;
> +        }
> +
> +        // XXX insert code to handle higher order....
> +        if (0) {
> +
> +        } else {
> +            arity = mr_convert::ToInt32(type_ctor_info->GetValue(0));
> +            // params = ???
> +        }
> +
> +        // args = params;
> +
> +	switch(arity) {
> +	case 0: 
> +                SUCCESS_INDICATOR = generic::generic_call2(
> +                	type_ctor_info->GetValue(1),
> +			X, Y);

It would be a good idea to assign type_ctor_info->GetValue(1) to a
named local variable.  I guess that "GetValue(1)" must be the field
which holds the unify procedure?  If so, it would be a very good idea
to define macros for the magic numbers like 1 and 3 above.

> +    static void
> +    mercury__builtin____Compare____int_0_0(
> +        MR_Word_Ref result, MR_Integer x, MR_Integer y)
> +    {
> +                    MR_COMPARE_LESS);
> +            MR_newobj(*result, r, 0);
> +    }

There seems to be a line or two missing there.

Better try compiling it ;-)

> +    static void
> +    mercury__builtin____Compare____float_0_0(
> +          MR_Word_Ref result, MR_Float x, MR_Float y)
> +    {
> +                              (MR_Runtime::MR_fatal_error(
> +                                 ""incomparable floats in compare/3""),
> +                              MR_COMPARE_EQUAL)); 
> +        MR_newobj(*result, r, 0);
> +    }

Likewise here.

> +    static int
> +    mercury__builtin__do_unify__int_0_0(MR_Box x, MR_Box y)
> +    {
> +            return mercury__builtin____Unify____int_0_0(
> +                    mr_convert::ToInt32(x), 
> +                    mr_convert::ToInt32(y)); 
> +    }

`mr_convert' should at least be `MR_convert'.

But even `MR_convert' is not a good name to use for a top-level
module.  Nor is `MR_Runtime'.  For a system that supports proper
namespaces, we should use them, rather than `MR_' prefixes.

Likewise we shouldn't be using `mercury__builtin__' prefixes,
instead it should be `mercury::builtin::' or something like that.

You may want to argue that it's not important to get the names right now. 
But my experience with getting the names right in the C runtime is
that it is much much better to get the names right as soon as
possible; anything else will only increase the pain.

> Index: library/exception.m
> +:- pragma foreign_decl("MC++", "
> +/* The `#ifndef ... #define ... #endif' guards against multiple inclusion */
> +#ifndef ML_DETERMINISM_GUARD
> +#define ML_DETERMINISM_GUARD
> +  #define zML_DET = 0
> +  #define zML_SEMIDET = 1
> +  #define zML_CC_MULTI = 2
> +  #define zML_CC_NONDET = 3
> +  #define zML_MULTI = 4
> +  #define zML_NONDET = 5
> +  #define zML_ERRONEOUS = 6
> +  #define zML_FAILURE = 7

What are all those zML_ macros for?
They're not syntactically correct anyway (the `=' is superfluous)
and they're not referenced.

> +	/*
> +	** The enumeration constants in this enum must be in the same
> +	** order as the functors in the Mercury type `determinism'
> +	** defined above.
> +	*/
> +	typedef enum {
> +		ML_DET,
> +		ML_SEMIDET,
> +		ML_CC_MULTI,
> +		ML_CC_NONDET,
> +		ML_MULTI,
> +		ML_NONDET,
> +		ML_ERRONEOUS,
> +		ML_FAILURE
> +	} ML_Determinism;

You should put a corresponding comment next to the `determinism' type.

> +#endif
> +").
> +
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is det,
> +			Det::out(bound(det))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_DET, 0);"
> +).
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is semidet,
> +			Det::out(bound(semidet))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_SEMIDET, 0);"
> +).
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is cc_multi,
> +			Det::out(bound(cc_multi))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_CC_MULTI, 0);"
> +).
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is cc_nondet,
> +			Det::out(bound(cc_nondet))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_CC_NONDET, 0);"
> +).
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is multi,
> +			Det::out(bound(multi))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_MULTI, 0);"
> +).
> +:- pragma foreign_code("MC++",
> +	get_determinism(_Pred::pred(out) is nondet,
> +			Det::out(bound(nondet))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_NONDET, 0);"
> +).
> +
> +:- pragma foreign_code("MC++",
> +	get_determinism_2(_Pred::pred(out, di, uo) is det,
> +			Det::out(bound(det))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_DET, 0);"
> +).
> +
> +:- pragma foreign_code("MC++",
> +	get_determinism_2(_Pred::pred(out, di, uo) is cc_multi,
> +			Det::out(bound(cc_multi))),
> +	will_not_call_mercury,
> +	"MR_newobj(Det, ML_CC_MULTI, 0);"
> +).

That code may soon all be wrong, when we change the way enums are
represented in the IL back-end.  Rather than using `MR_newobj', you
should use `MR_newenum' or something like that.

> +XXX :- external stops us from using this
> +
> +:- pragma foreign_code("MC++", builtin_throw(_T::in), [will_not_call_mercury], "
> +        mercury_exception *ex;
> +
> +        // XXX should look for string objects and set them as the message
> +
> +        if (false) {
> +            ex = new mercury_exception;
> +        } else {
> +            ex = new mercury_exception(""hello"");
> +        }
> +
> +        throw ex; 

The argument of the mercury_exception type should be `System.Object'
(or perhaps `Mercury.Object'), not `System.String'.

> Index: library/float.m

I notice that there are no test cases in our test suite for
several of the routines that you implemented MC++ version for,
including at least float__ceiling_to_int, float__floor_to_int,
float__round_to_int, and float__truncate_to_int.  It would be a good
idea to add some.

>  	% Maximum floating-point number
> -:- pragma c_code(float__max = (Max::out),
> +:- pragma foreign_code("C", float__max = (Max::out),
>  		[will_not_call_mercury, thread_safe],
>  	"Max = ML_FLOAT_MAX;").
> +:- pragma foreign_code("MC++", float__max = (Max::out),
> +		[will_not_call_mercury, thread_safe],
> +	"Max = Double::MaxValue;").

Is that System.Double?
Is `Double' the right type to use here?
I think you should probably be using a typedef of some kind,
e.g. `MR_Float'.

> +:- pragma foreign_code("MC++", float__min = (Min::out),
> +		[will_not_call_mercury, thread_safe],
> +	"Min = Double::MinValue;").

Likewise.

> +:- pragma foreign_code("MC++", float__epsilon = (Eps::out),
> +		[will_not_call_mercury, thread_safe],
> +	"Eps = Double::Epsilon;").

Likewise.

> Index: library/gc.m
> +:- pragma foreign_code("MC++", garbage_collect, [will_not_call_mercury], "
> +	// Do nothing 
>  ").

There should be an XXX there
(or a comment explaining why `Do nothing' is appropriate here).

> Index: library/int.m
> +:- pragma foreign_code("MC++", int__max_int(Max::out),
> +		[will_not_call_mercury, thread_safe], "
> +	Max = Int32::MaxValue;
> +").
> +
> +:- pragma foreign_code("MC++", int__min_int(Min::out),
> +		[will_not_call_mercury, thread_safe], "
> +	Min = Int32::MinValue;
> +").
> +
> +:- pragma foreign_code("MC++", int__bits_per_int(Bits::out),
> +		[will_not_call_mercury, thread_safe], "
> +	Bits = 32;
> +").
>
> +:- pragma foreign_code("MC++", int__quot_bits_per_int(Int::in) = (Div::out),
> +		[will_not_call_mercury, thread_safe], "
> +	Div = Int / 32;
> +").
> +
> +:- pragma foreign_code("MC++", int__times_bits_per_int(Int::in) = (Result::out),
> +		[will_not_call_mercury, thread_safe], "
> +	Result = Int * 32;
> +").

You shouldn't hard-code 32 in any of those places.

> Index: library/io.m
> @@ -1130,6 +1130,13 @@
>  	#endif
>  ").
>  
> +:- pragma foreign_code("MC++", "
> +	static MR_Word ML_io_stream_names;
> +	static MR_Word ML_io_user_globals;
> +	static int next_id;
> +").

Hmm...  you're relying here on foreign "MC++" code not being inlined
into different translation units (like we do with foreign "C" code).

I think at least it is worth a comment explaining why the MC++
version is different from the C version.

> +:- pragma foreign_code("MC++", 
> +		io__get_globals(Globals::uo, IOState0::di, IOState::uo), 
> +		will_not_call_mercury, "
> +	Globals = ML_io_user_globals;
> +	update_io(IOState0, IOState);
> +").
> +
> +:- pragma foreign_code("MC++", 
> +		io__set_globals(Globals::di, IOState0::di, IOState::uo), 
> +		will_not_call_mercury, "
> +	ML_io_user_globals = Globals;
> +	update_io(IOState0, IOState);
> +").

The `update_io' there should be `ML_update_io'.
Likewise for everywhere else in io.m and time.m.
And likewise for `initial_io_state' and `final_io_state'.

I guess those belong in a separate change.

> +#define MR_DownCast(Cast, Expr) dynamic_cast<Cast>(Expr)
> +#define MR_UpCast(Cast, Expr) ((Cast) (Expr))

Hmm, these macros are not very safe; they can be used to perform casts
of kinds other than the kind implied by the name.  The name expresses
the intent, which is good, but this intent is not enforced.

It may be worth mentioning this in a comment.

> +:- pragma foreign_code("MC++", "
>  
> +static MR_MercuryFile new_mercury_file(IO::Stream *stream, int line_number) {
> +	MR_MercuryFile mf = new MR_MercuryFileStruct();
> +	mf->stream = stream;
> +	mf->line_number = line_number;
> +	mf->id = next_id++;
> +	return mf;
> +}
> +
> +static MR_MercuryFile mercury_stdin =
> +	new_mercury_file(Console::OpenStandardInput(), 1);
> +static MR_MercuryFile mercury_stdout =
> +	new_mercury_file(Console::OpenStandardOutput(), 1);
> +static MR_MercuryFile mercury_stderr =
> +	new_mercury_file(Console::OpenStandardError(), 1);

Hmm, this will cause problems for GUI programs that don't want a
console window.  I think for Windows these streams should ideally be
created lazily, so that you don't get a blank console window if you're
not using it.

It would be OK to just add an XXX comment for now.

> +static MR_MercuryFile mercury_stdin_binary =
> +	new_mercury_file(0, 1);
> +static MR_MercuryFile mercury_stdout_binary =
> +	new_mercury_file(0, 1);
> +
> +static MR_MercuryFile mercury_current_text_input =
> +	new_mercury_file(Console::OpenStandardInput(), 1);
> +static MR_MercuryFile mercury_current_text_output =
> +	new_mercury_file(Console::OpenStandardOutput(), 1);
> +static MR_MercuryFile mercury_current_binary_input =
> +        new_mercury_file(0, 1);
> +static MR_MercuryFile mercury_current_binary_output =
> +        new_mercury_file(0, 1);

The current_* streams should point to the same objects as
mercury_stdin, mercury_stdout, etc., rather than being copies.

> +:- pragma foreign_code("MC++", "
> +
> +MR_MercuryFile
> +static mercury_open(MR_String filename, MR_String type)
> +{
> +        MR_MercuryFile mf = new MR_MercuryFileStruct();
> +        IO::FileMode fa;
> +        IO::Stream *stream;
> +
> +                // XXX get this right...
> +        if (type == ""r"") {
> +                fa = IO::FileMode::Open;
> +        } else if (type == ""w"") {
> +                fa = IO::FileMode::Append;
> +        } else {
> +                fa = IO::FileMode::OpenOrCreate;
> +        }

That looks very scary.  I would rather have a call to SORRY() than a
file open command that might get things wrong.

> +        stream = IO::File::Open(filename, fa);
> +
> +        if (!stream) {
> +                return 0;

s/0/NULL/

> +:- pragma foreign_code("MC++", "
> +
> +static void
> +mercury_print_string(MR_MercuryFile mf, MR_String s)
> +{
> +        IO::StreamWriter *w = new IO::StreamWriter(mf->stream);
> +        w->Write(s);
> +        w->Flush();

That is likely to be quite inefficient, I'd guess.
An XXX comment is warranted.

> +static void
> +mercury_print_binary_string(MR_MercuryFile mf, MR_String s)
> +{
> +        IO::StreamWriter *w = new IO::StreamWriter(mf->stream);
> +        w->Write(s);
> +        w->Flush();
> +}
> +
> +").

Likewise.

> +static void
> +mercury_close(MR_MercuryFile mf)
> +{
> +        if (mf != mercury_stdin &&
> +            mf != mercury_stdout &&
> +            mf != mercury_stderr)
> +        {
> +                mf->stream->Close();
> +                mf->stream = 0;
> +        }
> +}

s/0/NULL/

> +:- pragma foreign_code("MC++", 
> +	io__write_char(Character::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "
> +	if (Character == '\\n') {
> +		mercury_current_text_output->line_number++;
> +	}
> +	update_io(IO0, IO);
> +").

What happened to the code to actually output the character?

> +:- pragma foreign_code("MC++",
> +	io__write_int(Val::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_text_output->stream);
> +	w->Write(Val.ToString());
> +	w->Flush();
> +	update_io(IO0, IO);
> +").
>
> +:- pragma foreign_code("MC++",
> +	io__write_float(Val::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_text_output->stream);
> +	w->Write(Val.ToString());
> +	w->Flush();
> +	update_io(IO0, IO);
> +").

Any reason why you don't use mercury_print_string() for
these?

> +:- pragma foreign_code("MC++",
> +	io__write_byte(Byte::in, _IO0::di, _IO::uo),
> +		[may_call_mercury, thread_safe], "
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_text_output->stream);
> +	w->Write(Byte.ToString());
> +	w->Flush();
> +").

That looks quite wrong.  Bytes shouldn't be converted to strings.

> +:- pragma foreign_code("MC++",
> +	io__write_string(Stream::in, Message::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], 
> +"{
> +	MR_MercuryFile stream = MR_DownCast(MR_MercuryFile, 
> +		MR_word_to_c_pointer(Stream));
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_binary_output->stream);
> +	w->Write(Message);
> +	w->Flush();
> +	update_io(IO0, IO);
> +}").

Any reason why you don't use mercury_print_string() here?

> +:- pragma foreign_code("MC++",
> +	io__write_int(Stream::in, Val::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "{
> +	MR_MercuryFile stream = MR_DownCast(MR_MercuryFile, 
> +		MR_word_to_c_pointer(Stream));
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_binary_output->stream);
> +	w->Write(Val.ToString());
> +	w->Flush();
> +	update_io(IO0, IO);
> +}").
> +
> +:- pragma foreign_code("MC++",
> +	io__write_float(Stream::in, Val::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "{
> +	MR_MercuryFile stream = MR_DownCast(MR_MercuryFile, 
> +		MR_word_to_c_pointer(Stream));
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_binary_output->stream);
> +	w->Write(Val.ToString());
> +	w->Flush();
> +	update_io(IO0, IO);
> +}").

Likewise.

> +:- pragma foreign_code("MC++",
> +	io__write_byte(Stream::in, Byte::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "{
> +	MR_MercuryFile stream = MR_DownCast(MR_MercuryFile, 
> +		MR_word_to_c_pointer(Stream));
> +	IO::StreamWriter *w = new IO::StreamWriter(
> +		mercury_current_binary_output->stream);
> +	w->Write(Byte.ToString());
> +	w->Flush();
> +	update_io(IO0, IO);
> +}").

That looks wrong again.

> +:- pragma foreign_code("MC++",
> +	io__flush_output(Stream::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "{
> +	MR_MercuryFile stream = MR_DownCast(MR_MercuryFile, 
> +		MR_word_to_c_pointer(Stream));
> +	stream->stream->Flush();
> +	update_io(IO0, IO);
> +}").
> +
> +:- pragma foreign_code("MC++",
> +	io__flush_binary_output(_Stream::in, IO0::di, IO::uo),
> +		[may_call_mercury, thread_safe], "{
> +	MR_Runtime::SORRY(""foreign code for this function"");
> +	update_io(IO0, IO);
> +}").

Any reason why the code for this needs to be different
to the code for io__flush_output?

[... to be continued ...]

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