[m-dev.] Re: pragma fact_table

[Fergus Henderson]

David Matthew OVERTON, you wrote:
> 
> compiler/inlining.m:
> 	Stop `model_non' `pragma c_code' being inlined unless it hash

s/hash/has/

> -		{
> -			Float f = %s, g, h;
> -			if (f <= 0.0) f = -f;
> -			if (f == 0.0) f = 1e-15;
> -			h = log(f);
> -			h = ceil(h);
> -			g = exp(h);
> -			g = f / g * 2147483647.0;
> -			if (h <= 0.0) h = -h;
> -			hashval = (Integer)g %% hashsize;
> -			hashval = ((Integer)h + 31*hashval) %% hashsize;
> -		}
> +		hashval = hash_float(%s) %% hashsize;

You should not assume that the result of hash_float is non-negative.

> -				ind = %s.index;
> +				ind = (Word)%s.index;

Put a space after the ')'.

>  generate_fact_lookup_code(PredName, [pragma_var(_, VarName, Mode)|PragmaVars],
> +		[Type | Types], ModuleInfo, ArgNum, FactTableSize, C_Code) :-
>  	NextArgNum is ArgNum + 1,
> +	( mode_is_fully_output(ModuleInfo, Mode) ->
>  		C_Code_Template = 
> +		"\t\t%s = %smercury__%s_fact_table[ind/%d][ind%%%d].V_%d;\n",
> +		( Type = term__functor(term__atom("string"), [], _) ->
> +			% Cast ConstString -> Word -> String to avoid 
> +			% gcc warning "assignment discards `const'".
> +			% XXX Is this safe or should I be copying the string
> +			% onto the heap?
> +			Cast = "(String)(Word)"

Put a space after each ')'.

The code is potentially unsafe if the user used a `uo' mode.
Since mode_is_fully_output allows `uo' modes, yes, it is unsafe.
Someone might want to write code which destructively updates strings, e.g.

	:- func to_upper(string::di) = (string::uo) is det.
	:- pragma c_code(to_upper(S0) = S), will_not_call_mercury, "{
		int i;
		for (i = 0; i < strlen(S0); i++) {
			S0[i] = toupper(S0);
		}
		S = S0;
	}").
		
On the other hand you don't want to copy to the heap in the normal `out'
mode case.  So you should use inst_is_not_partly_unique to check.

But even in the non-unique case, there is still a possible problem --
you should use make_aligned_string() from runtime/mercury_string.h to
ensure that strings are properly aligned.

By the way, the code we currently generate for

	:- pred p(string::uo).
	p("blah").

is also unsafe in the same sense -- we need to fix that before
we claim to support unique modes.

> +	% Convert an rval (represented as a string), from a C type to
> +	% a mercury C type. (ie. convert strings and floats to words).
> +:- pred convert_type_to_mercury(string, type, string).
> +:- mode convert_type_to_mercury(in, in, out) is det.
> +
> +	% Convert an rval (represented as a string), from a mercury C type to
> +	% a C type. (ie. convert words to strings and floats if required).
> +:- pred convert_type_from_mercury(string, type, string).
> +:- mode convert_type_from_mercury(in, in, out) is det.

Insert "Generate C code to ..." at the start of and append "and return
the resulting C code as a string" at the end of both of those comments.

> +			( { AssumeGmake = no} ->
> +				io__write_strings(DepStream,
> +					[ModuleName, ".fact_tables.os = "]),
> +				write_dependencies_list(FactDeps, ".o",
> +					DepStream),
> +				io__write_strings(DepStream, [
> +					"\n\n", ModuleName, 
> +					".fact_tables.cs = $(", ModuleName,
> +					".fact_tables.os:.o=.c)\n\n"
> +				])

That code assumes GNU make.  The `$(blah:.o=.c)' syntax is not portable
to other version of make.

Can I please see another diff for fact_table.m relative to the version
as at this diff, and another diff for modules.m?

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.