bug#8545: issues with recent doprnt-related changes

[Eli Zaretskii]

> Date: Wed, 27 Apr 2011 16:51:06 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 8545@debbugs.gnu.org
> 
> >> A quick second scan found a minor bug in size parsing: the
> >> expression "n >= SIZE_MAX / 10" should be "n > SIZE_MAX / 10".
> > 
> > When they get to messages as long as SIZE_MAX, let them sue me for
> > taking away one byte.
> 
> It's not a question of saving space at run-time.  It's a question of
> helping the reader.  The reader is left wondering: why is that ">="
> there?

The reader will be wondering with ">" as well.  There's a comment
about checking for overflow which should be a good hint, especially
since SIZE_MAX is compared against.

>  And why is there another test "n * 10 > SIZE_MAX - (fmt[1] -
> '0')" that always returns 0, no matter what?

??? What happens if n*10 is SIZE_MAX-1 and fmt[1] is '2'?  Is the
result still zero?

>   /* Avoid int overflow, because many sprintfs seriously mess up
>      with widths or precisions greater than INT_MAX.  Avoid size_t
>      overflow, since our counters use size_t.  This test is slightly
>      conservative, for speed and simplicity.  */
>   if (n >= min (INT_MAX, SIZE_MAX) / 10)
>      error ("Format width or precision too large");

Sorry, I don't see how this is clearer.  The current code after the
test is built out of the same building blocks as the test, and
therefore the intent and the details of the test are easier to
understand than with your variant, which perhaps is mathematically and
numerically equivalent, but makes the code reading _harder_ because it
severs the syntactical connection between the two.

> > "MOST_POSITIVE_FIXNUM + 1" is too much, since MOST_POSITIVE_FIXNUM
> > should be able to cover the terminating null character in Emacs.
> 
> Why?  Emacs size fields count the bytes in the string, and does not
> count the terminating null byte (which is not part of the string).

That's not what I know.  String positions are zero-based and extend to
include the terminating null character.  See the relevant parts of the
display engine code.

> * doprnt invokes strlen to find the length of the format.  The
>   vsnprintf code didn't need to do that: it traversed the format once.
>   Surely it shouldn't be hard to change doprnt so that it traverses
>   the format once rather than twice.

doprnt is invoked in the context of displaying an error message that
throws to top level, and so it doesn't need to be optimized (which
will surely make the code more complex and error-prone, and its
maintenance harder).

> * Sometimes verror will incorrectly truncate a string, even when there
>   is plenty of memory.  verror might call doprnt (buffer, SIZE, m, m +
>   mlen, ap), and doprnt might discover that a multibyte character is
>   chopped in half at the end of the output buffer, and might return
>   (say) SIZE - 2.  verror will incorrectly conclude that the output
>   was just fine, but it wasn't complete.

Not an issue, what with the initial buffer size you enlarged to 4000.
I needed to artificially lower it to just 2 dozen bytes, just to see
the recovery code in action.  If someone wants to display a 4001-byte
message that ends with a multibyte non-ASCII character, let them be
punished for not knowing how to write concisely.

> * verror might invoke doprnt two or more times, which means that
>   doprnt will traverse ap twice.  This does not work in general; the C
>   standard is quite clear that the behavior is undefined in this case.

Are there any platforms supported by Emacs where this actually
happens?  If not, let's forget about this issue until it hits us.

I'm closing this bug.  We are already well past any real problems, and
invested too much energy and efforts of two busy people on this tiny
function, all because of your stubborn insistence on using a library
function where it doesn't fit the bill.  I hope you now have more
respect for views and code of others in general, and mine in
particular, so we won't need to go through this painful experience
again in the future.

Let's move on; I still need to work on the bidirectional display of
overlay strings and display properties, a job that was already delayed
by several precious days due to these disputes and the gratuitous work
on the code that should have been left alone in the first place.