[m-dev.] for review: string__fmt
Peter Ross
peter.ross at miscrit.be
Thu Aug 10 18:37:17 AEST 2000
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
--------------------------------------------------------------------------
More information about the developers
mailing list