[m-dev.] for review: pragma foreign_code for MC++ (part 2/2)

[Fergus Henderson]

On 10-Nov-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> +:- pred mercury_output_foreign_language_string(foreign_language::in,
> +		io__state::di, io__state::uo) is det.
> +mercury_output_foreign_language_string(c) -->
> +	io__write_string("""C""").
> +mercury_output_foreign_language_string(managedcplusplus) -->
> +	io__write_string("""MC++""").

There seems to be some code duplication here; it would be better for
this to use the foreign_language_string/1 function defined in globals.m.

> Index: compiler/ml_code_gen.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/ml_code_gen.m,v
> retrieving revision 1.64
> diff -u -r1.64 ml_code_gen.m
> --- compiler/ml_code_gen.m	2000/11/08 07:23:06	1.64
> +++ compiler/ml_code_gen.m	2000/11/08 11:25:30
> @@ -720,7 +720,8 @@
>  
>  :- import_module ml_type_gen, ml_call_gen, ml_unify_gen, ml_switch_gen.
>  :- import_module ml_code_util.
> -:- import_module arg_info, export, llds_out. % XXX needed for pragma C code
> +:- import_module llds_out. % XXX needed for pragma C code
> +:- import_module arg_info, export, foreign.

The import of `arg_info' should still have an XXX next to it,
since arg_info is specific to the LLDS back-end.
Likewise for the import of `export' and `foreign',
since currently they still depend on the LLDS back-end.

> +++ compiler/mlds_to_c.m	2000/11/10 06:51:02
> +:- func this_file = string.
> +this_file = "mlds_to_c.m".

I suggest you add an `end_module' declaration here.

> Index: compiler/options.m
> +		"--use-foreign-language <foreign language>",
> +		"\tUse the given foreign language to implement predicates",
> +		"\twritten in foreign languages.  Any name that can be used",
> +		"\tto specify foreign languages in pragma foreign declarations",
> +		"\tis valid, but not all foreign languages are implemented",
> +		"\tin all backends.",
> +		"\tDefault value is C for the LLDS and MLDS->C backends,",
> +		"\tor ManagedC++ for the .NET backend.",

The default values should be in `single quotes'.

The documented default value for the .NET backend should match the
actual default used (here you say "ManagedC++", but the value used
in the source code is "MC++").

The option should be documented in doc/user_guide.texi.

> +++ compiler/prog_data.m	2000/11/10 06:20:33
...
> +	% Convert the attributes to their source code representations.
> +	% Useful if you need to write a pragma out to a file.
> +:- pred attributes_to_strings(pragma_foreign_code_attributes::in,
> +		list(string)::out) is det.
...
> +attributes_to_strings(Attrs, StringList) :-
> +	Attrs = attributes(_Lang, MayCallMercury, ThreadSafe, TabledForIO),

Why is _Lang ignored here?

If that is deliberate, then it should be mentioned (and explained)
in the routine's documentation.

> Index: compiler/prog_io_pragma.m
...
>  parse_pragma_type(ModuleName, "import", PragmaTerms,
>  			ErrorTerm, _VarSet, Result) :-
>  	(
>  	    (
>  		PragmaTerms = [PredAndModesTerm, FlagsTerm, C_FunctionTerm],
> -		( parse_pragma_c_code_attributes_term(FlagsTerm, Flags) ->
> +			% XXX we assume all imports are C
> +		( parse_pragma_foreign_code_attributes_term(c,
> +				FlagsTerm, Flags) ->
>  			FlagsResult = ok(Flags)
>  		;
>  			FlagsResult = error("invalid second argument in `:- pragma import/3' declaration -- expecting C code attribute or list of attributes'",
> @@ -353,7 +389,7 @@
>  	        )
>  	    ;
>  		PragmaTerms = [PredAndModesTerm, C_FunctionTerm],
> -		default_attributes(Flags),
> +		default_attributes(c, Flags),

It would be better to add

	Language = c,

at the top, and put the XXX comment next to that line,
rather than making the same assumption twice.

> --- tests/invalid/pragma_c_code_and_clauses1.err_exp	1998/02/02 03:01:11	1.3
> +++ tests/invalid/pragma_c_code_and_clauses1.err_exp	2000/11/09 00:58:23
> @@ -1,6 +1,6 @@
>  pragma_c_code_and_clauses1.m:009: Warning: `pragma' declaration in module interface.
>  pragma_c_code_and_clauses1.m:007: Warning: clause in module interface.
> -pragma_c_code_and_clauses1.m:009: Error: `:- pragma c_code' declaration for predicate `pragma_c_code_and_clauses1:foo/1'
> +pragma_c_code_and_clauses1.m:009: Error: `:- pragma foreign_code' declaration for predicate `pragma_c_code_and_clauses1:foo/1'

The updated error messages are misleading, since there's no
`pragma foreign_code' in the test case's source code.

This could be fixed by keeping track of the syntax used in
the original declaration.

Alternatively, the error messages could be changed so that they all
say "`pragma foreign_code' (or `pragma c_code')".

Apart from that, this change looks fine.
As usual I'd like to see a relative diff for any new changes.

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