From: Brian Koropoff <bkoropoff@gmail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: dash@vger.kernel.org
Subject: Re: [PATCH 1/2] Fix additional use of 'j' printf length modifier.
Date: Sat, 16 Apr 2011 11:25:19 -0700 [thread overview]
Message-ID: <1302978319.19920.14.camel@gemini> (raw)
In-Reply-To: <20110416045435.GA10455@elie>
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
next prev parent reply other threads:[~2011-04-16 18:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-16 2:30 Fix for Solaris patch and new HP-UX patch Brian Koropoff
2011-04-16 2:34 ` [PATCH 1/2] Fix additional use of 'j' printf length modifier Brian Koropoff
2011-04-16 4:54 ` Jonathan Nieder
2011-04-16 18:25 ` Brian Koropoff [this message]
2011-07-07 6:41 ` Herbert Xu
2011-07-07 7:12 ` Jonathan Nieder
2011-07-07 7:32 ` Brian Koropoff
2011-07-07 7:44 ` Herbert Xu
2011-04-16 2:34 ` [PATCH 2/2] Port to HP-UX Brian Koropoff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1302978319.19920.14.camel@gemini \
--to=bkoropoff@gmail.com \
--cc=dash@vger.kernel.org \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).