[m-dev.] for review: string__fmt

[Fergus Henderson]

On 09-Aug-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> 
> Here is the new and improved version of string__format.  If you review
> it now you will get a free set of steak knives.

I want those steak knives ;-)

> 		String = format_string(
> 				format_string(Flags, Width, Prec, "s"), Str)

The overloading of `format_string' to have two different meanings
depending on the arity is not a good idea, IMHO.
I suggest you rename the /4 version

> 	% Construct a format string suitable to passing to sprintf.
> :- func format_string(list(char), maybe(list(char)),
> 		maybe(list(char)), string) = string.

as e.g. `make_format'.

> 	% Create a string from a int using the format string.
> 	% Note is is the responsibility of the caller to ensure that the
> 	% format string is valid.
> :- func format_int(string, int) = string.
> :- pragma c_code(format_int(FormatStr::in, Val::in) = (Str::out),
> 		[will_not_call_mercury, thread_safe], "{
> 	Str = MR_make_string(MR_PROC_LABEL, FormatStr, (long) Val);
> }").

The cast to `long' here is problematic -- it will break on the Win64 port,
where `MR_Integer' will be 64 bits, but `long' is 32 bits.

At very least there should be an XXX here.

> +:- type spec
> +		% valid integer specifiers
> +	--->	d(int)
> +	;	i(int)
> +	;	o(int)
> +	;	u(int)
> +	;	x(int)
> +	;	cX(int)

The meaning of "cX" was a mystery to me for a while.
At very least there should be a comment explaining it.
But I think it would make more sense to use "'X'"
rather than "cX".

> +		% valid float specifiers
> +	;	e(float)
> +	;	cE(float)
> +	;	f(float)
> +	;	cF(float)
> +	;	g(float)
> +	;	cG(float)

Likewise for these.

> +++ mercury_string.c	Thu Aug 10 00:46:53 2000
> +MR_String
> +MR_make_string(MR_Code *proclabel, const char *fmt, ...) {
> +	va_list		ap;
> +	MR_String	result;
> +	int 		n;
> +
> +#ifdef HAVE_VSNPRINTF
> +	/* Guess that 100 bytes should be sufficient */
> +	int 		size = 100;
> +	char		*p;
> +	
> +	p = MR_NEW_ARRAY(char, size);
> +
> +	while (1) {
> +		/* Try to print in the allocated space. */
> +		va_start(ap, fmt);
> +		n = vsnprintf(p, size, fmt, ap);
> +		va_end(ap);
> +
> +		/* If that worked, return the string.  */
> +		if (n > -1 && n < size)
> +			 continue;
> +
> +		/* Else try again with more space.  */
> +		if (n > -1)    /* glibc 2.1 */
> +			 /* precisely what is needed */
> +			 size = n+1;
> +		else           /* glibc 2.0 */
> +			size *= 2;  /* twice the old size */
> +
> +		MR_RESIZE_ARRAY(p, char, size);
> +	}
> +#else
> +		/* It is possible for this buffer to overflow	*/
> +		/* and then bad things may happen		*/
> +	char p[40960];
> +
> +	va_start(ap, fmt);
> +	n = vsprintf(p, fmt, ap);
> +	va_end(ap);
> +#endif
> +	      
> +	MR_allocate_aligned_string_msg(result, strlen(p),
> +			proclabel);
> +	strcpy((char *) result, p);
> +	MR_free(p);
> +	return result;

This code has a number of problems, and has clearly not been tested.

	- The while loop can never terminate; probably the `continue'
	  should be a `break'.

	- For the `#else' case, you call `MR_free(p)' but `p' is a local
	  variable allocated on the stack

Also, if you're trying to optimize the code to avoid memory allocation,
then in the case where we have vnsprintf(), it would be better if
the first try used a fixed-size array allocated on the stack;
this would avoid one dynamic memory allocation in the common case.

Please use curly braces for the bodies of `if' and `else'
statements.  Please put spaces around operators such as `+'.

> +/* void MR_allocate_aligned_string_msg(MR_ConstString &ptr, size_t len,
> +**		Code *proclabel, const char *type);
> +** Allocate enough word aligned memory to hold len characters.  Also
> +** record for memory profiling purposes the location, proclabel, of the
> +** allocation if profiling is enabled.
> +**
> +** BEWARE: this may modify `hp', so it must only be called from
> +** places where `hp' is valid.  If calling it from inside a C function,
> +** rather than inside Mercury code, you may need to call
> +** save/restore_transient_hp().
> +*/
> +#define MR_allocate_aligned_string_msg(ptr, len, proclabel)		\
...
> +/*
> +** Return an MR_String which has been created using the format string,
> +** fmt, passed to sprintf.  If memory profiling is turned on, record the
> +** allocation as coming from proclabel.
> +*/
> +MR_String MR_make_string(MR_Code *proclabel, const char *fmt, ...);

The documentation for this routine should explain whether the caller
needs to deallocate the string afterwards, and if so, how.
Also the documentation for this routine should have the same
"BEWARE:" notice as for `MR_allocate_aligned_string_msg', I think.

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