[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#8545: issues with recent doprnt-related changes
From: |
Paul Eggert |
Subject: |
bug#8545: issues with recent doprnt-related changes |
Date: |
Sun, 24 Apr 2011 22:46:33 -0700 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 |
This is a followup to Bug#8435. Eli invited me to review the recent
doprnt-related changes, so here's a quick review:
* doprnt returns size_t. But Stefan wrote that he prefers sizes to be
signed values, and doprnt always returns a value that can fit in
EMACS_INT. So shouldn't doprnt return EMACS_INT, as it did before?
* doprnt supports only a small subset of the standard printf formats,
but this subset is not documented. It's unclear what the subset is.
Or it's a superset of the subset, with %S and %l? Anyway, this
should be documented clearly in the lead comment.
* I suggest that every feature in doprnt be a feature that is actually
needed and used; this will simplify maintainance.
* Format strings never include embedded null bytes, so there's
no need for doprnt to support that.
* If the format string is too long, the alloca inside doprnt will
crash Emacs on some hosts. I suggest removing the alloca,
instituting a fixed size limit on format specifiers, and documenting
that limit. Since user code cannot ever supply one of these
formats, that should be good enough.
* The width features of doprnt (e.g., %25s) are never used. That part
of the code is still buggy; please see some comments below. I
suggest removing it entirely; this will simplify things. But if not:
- doprnt mishandles format specifications such as %0.0.0d.
It passes them off to printf, and this results in undefined
behavior, near as I can tell.
- doprnt uses atoi (&fmtcpy[1]), but surely this isn't right if
there are flags such as '-'.
- Quite possibly there are other problems in this area, but I
didn't want to spend further time reviewing a never-used feature.
* In this code, in verror:
used = doprnt (buffer, size, m, m + mlen, ap);
/* Note: the -1 below is because `doprnt' returns the number of bytes
excluding the terminating null byte, and it always terminates with a
null byte, even when producing a truncated message. */
if (used < size - 1)
break;
I don't see the reason for the "- 1". If you replace this with:
used = doprnt (buffer, size, m, m + mlen, ap);
if (used < size)
break;
the code should still work, because, when used < size, the buffer
should be properly null-terminated. If it isn't then there's something
wrong with doprnt, no?
* In this code, in verror:
else if (size < size_max - 1)
size = size_max - 1;
there's no need for the "- 1"s. Just use this:
else if (size < size_max)
size = size_max;
* This code in verror:
if (buffer == buf)
buffer = (char *) xmalloc (size);
else
buffer = (char *) xrealloc (buffer, size);
uses xrealloc, which is unnecessarily expensive, as it may copy the
buffer's contents even though they are entirely garbage here. Use
this instead, to avoid the useless copy:
if (buffer != buf)
xfree (buffer);
buffer = (char *) xmalloc (size);
- bug#8545: issues with recent doprnt-related changes,
Paul Eggert <=
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/25
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/26
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Juanma Barranquero, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Juanma Barranquero, 2011/04/28