Hello,
On Fri, 2011-04-15 at 23:54 -0500, Jonathan Nieder wrote:
> Hi,
>
> Brian Koropoff wrote:
>
> > The printf builtin modifies the user's format strings
> > by prefixing integer conversion specifications with the
> > 'j' (intmax_t) length modifier. Since this is not portable,
> > instead prefix them with the length modifier extracted from
> > the PRIdMAX constant.
>
> This assumes PRIdMAX, PRIxMAX, etc all consist of the same prefix
> before the standard characters. Since the most common definitions
> are j<usual char>, l<usual char>, q<usual char>, I64<usual char>,
> and ll<usual char>, that's probably a safe assumption. I wonder why
> C99 and its predecessors did not use
>
> printf("%"PRIMAX"x\n", val);
To be completely safe we could refactor mklong() to take the PRI?MAX
string as a parameter and use the correct one at each invocation site.
I'm not sure this is warranted until we actually encounter a platform
where it makes a difference.
> Oh well. Maybe it would warrant a comment, though?
>
> /*
> * Replace a string like
> *
> * %92.3u
> * ^ ^--- ch
> * '-------- str
> *
> * with "%92.3" PRIuMAX "".
> *
> * Although C99 does not guarantee it, we assume PRIiMAX,
> * PRIoMAX, PRIuMAX, PRIxMAX, and PRIXMAX are all the same
> * as PRIdMAX with the final 'd' replaced by the corresponding
> * character.
> */
>
> > --- a/src/bltin/printf.c
> > +++ b/src/bltin/printf.c
> > @@ -317,15 +317,16 @@ static char *
> > mklong(const char *str, const char *ch)
> > {
> > char *copy;
> > - size_t len;
> > + size_t len;
> > + size_t pridmax_len = strlen(PRIdMAX);
>
> I think just using strlen(PRIdMAX) as-is would make it clearer that we
> are expecting the compiler to inline the "strlen" and provides a
> reminder of the value, too (i.e., is it 2 or 3 for "jd"?).
>
> >
> > - len = ch - str + 3;
> > + len = ch - str + pridmax_len;
>
> This changes the meaning of "len" to no longer be the size of the
> buffer. I suppose that doesn't matter, but...
>
> > STARTSTACKSTR(copy);
> > - copy = makestrspace(len, copy);
> > - memcpy(copy, str, len - 3);
> > - copy[len - 3] = 'j';
> > - copy[len - 2] = *ch;
> > - copy[len - 1] = '\0';
> > + copy = makestrspace(len + 1, copy);
> > + memcpy(copy, str, len - pridmax_len);
> > + memcpy(copy + len - pridmax_len, PRIdMAX, pridmax_len - 1);
> > + copy[len - 1] = *ch;
> > + copy[len] = '\0';
>
> ... the arithmetic is getting complicated. I think mempcpy could make
> the intention clearer, like so.
>
> char *p;
> [...]
> len = ch - str + strlen(PRIdMAX) + 1;
> p = copy = makestrspace(len, copy);
> p = mempcpy(p, str, ch - str);
> p = mempcpy(p, PRIdMAX, strlen(PRIdMAX) - 1);
> *p++ = *ch;
> *p++ = '\0';
>
> Like this, maybe (on top, untested)?
I like this, it feels a bit cleaner. Maybe get rid of the len variable
now since it's only used once.
-- Brian
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
