[m-dev.] for review: string__fmt

[Peter Ross]

On Thu, Aug 10, 2000 at 06:02:58AM +1000, Fergus Henderson wrote:
> 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'.
> 
Ok.

> > 	% 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.
> 
Damn, I thought that would be sufficient.  I have added an XXX.

> > +:- 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".
> 
I tried that but in some spots it was still being parsed as a variable.
I will try an reconstruct it as a test case.  For the moment a comment
has been added.

> > +		% 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.
> 
Caught me out, just did some minor layout changes to remove some code
duplication.  Impossible to make mistake.

> 	- The while loop can never terminate; probably the `continue'
> 	  should be a `break'.
> 
Yes, I discovered that one last night while bootchecking.

> 	- For the `#else' case, you call `MR_free(p)' but `p' is a local
> 	  variable allocated on the stack
> 
Also fixed last night, note that when HAVE_VSPRINTF is defined it is
dynamically allocated memory.

> 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.
> 
Good idea.  After the little continue disaster I will commit what I have
now and then do this change as a seperate change.

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

Here is the revised code

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) {
			 break;
		}

		/* 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);

#ifdef HAVE_VFPRINTF
	MR_free(p);
#endif

	return result;
}

> > +/* 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.
> 
Added the BEWARE: notice and remarked that the memory is allocated on
the mercury heap using MR_allocate_aligned_string_msg

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