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

[Fergus Henderson]

On 18-Dec-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> On 05-Dec-2000, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > On 04-Dec-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> > > +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.
> 
> Yes.
> 
> If you want to embed a resource in a .dll, then you give the option
>         /embed:filename
> 
> If you want to give it a specific name, you have to do
>         /embed:filename,resourcename
> 
> And if you want to make it publically accessible, you have to add a ,Y
>         /embed:filename,resourcename,Y
> 
> GNU Make doesn't allow $(patsubst ... ) rules to do double subtitutions
> (the pattern I want is %.dll,%,Y).
> 
> Microsoft's option handling doesn't allow you to omit the resourcename
> if you want to give a ,Y option.  So you *have* to do a double
> substitution (hence the use of foreach instead of a simple pattern style
> substitution) and you have to put commas in the "text" portion of the
> foreach.

Tyse, Tyse, you sound very defeatist.
All this working on Microsoft products must be getting you down ;-)

You don't have to put commas in the "text" portion of foreach.
You could use e.g. the following

	ALL_DLLS_BASE  = $(ALLDLLS:%.dll=%)
	EMBED_ALL_DLLS = $(foreach dll_name,$(ALL_DLLS_BASE),$(EMBED_ONE_DLL))
	EMBED_ONE_DLL  = /embed:$(dll_name).dll,$(dll_name),Y

which puts the commas in a separate variable.
This technique is even recommended by the GNU Make manual:

|  When TEXT is complicated, you can improve readability by giving it a
|  name, with an additional variable:

Or you could use $(shell) and sed.  Or you could even write a C
program to generate that part of the Makefile.  There's lots of
possibilities.

> GNU Make says nothing about embedded commas (and possible escaping of
> them if it were a problem) but it also never mentions that pattern
> subsitutions are restricted to a single subsitution.
> 
> > > +
> > > +mercury.dll: $(ALLDLLS)
> > > +	al -out:mercury.dll $(ALLDLLS)
> > 
> > If you bother to autoconfigure ILASM, shouldn't you also autoconfigure "al"?
> 
> I only bothered with ILASM to find out the SDK path, so I wasn't going
> to bother with al just yet.

OK.  But it might be good to just define

	AL = al

and then use $(AL) rather than hard-coding `al'.  It probably doesn't
make much difference in this case, but in general it's good style to
avoid hard-coding such names whenever possible.

> > > +:- 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"?
> 
> I got 'method not found' errors.
> 
> It still happens.  I don't know why -- it doesn't seem to find
> 'Diagnostics'.  I will have to spend some time looking into it at some
> point in the future.

Fine, just make the comment clearer.
It's not clear from the comment what the words
"this" and "here" refer to.

> > >  :- 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.
> 
> Can't do that, the MC++ compiler barfs.  
> 
> Another SORRY here until I can look into it futher.

Fine.  Just add an XXX comment explaining it.

> > The numbering of the generic_res_call functions is odd;
> > why is it called `call3', rather than `call0'?
> 
> Because it implements a generic call of an arity 3 procedure with a
> result.  It's a little like call/3.

call/3 has three arguments, but generic_res_call3 has *four*
arguments.  Isn't it more like call/4?

> > > +:- 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?
> 
> Yes, but it does shallow copies.  There are some Clone() methods that do
> 

That do what?
That do shallow copies?

If so, please add an XXX comment mentioning that.

> I've fixed mr_convert and MR_Runtime.  They are now
> mercury::runtime::convert and mercury::runtime::errors.
> 
> I've also replaced the mercury__builtin__ with mercury::builtin::

Fantastic, thanks.

> > >  	% 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'.
> 
> Sounds like a great idea.  The MC++ compiler won't accept it thought.
> I can get it to give an internal compiler error, or it complains that
> mercury::MR_BoxedFloat isn't a class/struct/union.

How about a #define?

> > > +:- 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.
> 
> Either I don't understand this comment, or you are confusing
> foreign_code with foreign_decls.
>
> foreign_code cannot be inlined into multiple different translation units.
> It can only appear once.  It has to be put into a single
> translation unit.  No other foreign_code can rely on it being put into
> it's own translation unit (and it could be said that I'm breaking this
> rule because I don't write a separate decl).
> 
> foreign_*decls* can be inlined into multiple different translation units.

I was worried about the foreign_code/4 (i.e. foreign_proc) procedures
that reference that definition being inlined into other translation
units in which the definition is not visible, not about that
definition itself being inlined.  Don't you need a separate
foreign_decl of some kind to handle that case?

Are those those declarations declaring static class members,
or are they static variables at namespace scope?

> > > +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.
> 
> I'll XXX that too.  It's not trivial to arrange this.
> 
> > 
> > > +:- 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.
> 
> Then a SORRY you shall get.
> 
> I did mention that all this stuff is experimental right, and will as
> likely delete all your files as work? 
> 
> > > +:- 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.
> 
> Ok.
> 
> > 
> > > +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/
> 
> This would of course involve #defining NULL.
> 
> It's not currently defined because I'm deliberatley not using the
> "standard" system libraries (just in case they drag in more non .NET
> stuff).

Could you explain that rationale in a bit more detail?
What would be wrong with dragging in more non .NET stuff?
(I don't disagree with you, I'd just like to understand
the issue better.)

<stddef.h> should be pretty safe in that respect.
In ANSI/ISO C and POSIX, it only defines the following:

	macro     NULL
	macro     offsetof()
	typedef   ptrdiff_t
	typedef   size_t
	typedef   wchar_t

ANSI/ISO C requires that system headers like <stddef.h> do not
drag in stuff from other header files into the user's namespace.

You could always use MR_NULL, I suppose.

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