bug#43439: [PATCH] doprnt improvements

[Paul Eggert]

On 9/16/20 7:58 AM, Eli Zaretskii wrote:

> Emacs traditionally supports strings with
> embedded null characters, and this feature is in line with that.  It
> is true that it is currently unused, but why is it a good idea to
> remove it?

It's a good idea not only because the feature is currently unused and its 
support complicates and adds bugs to the code, but also because it would be a 
bad idea to ever use the feature.

The Emacs feature is for Lisp strings. Emacs does not (and for API reasons, it 
cannot practically) rely on embedded NULs in C strings. Among other things, if 
we tried to use C-style printf formats with embedded NULs, GCC's warnings about 
formats not matching their arguments would stop working. These GCC warnings are 
quite useful for preventing bugs in Emacs's C code and have helped to catch many 
such bugs, and we should not give them up.

More generally, the vestigial support for NULs and %S in doprnt's C formats 
dates back to long ago, before GCC warned about these features. It may have made 
sense back then but it does not make sense now. Any C-level formatting facility 
that supports NULs and %S should not attempt to use the longstanding printf API 
that is incompatible with such support - it should be a separate facility. And 
Emacs C code already has a non-printf facility for formatting with NULs and %S - 
e.g., Fformat and Fformat_message - so it doesn't need yet another one.

> If the problem is the slight inefficiency caused by the call to
> strlen, we could instead solve it in the callers: all the formats I've
> seen are const strings, so the value of FORMAT_END can be computed at
> compile time, and used instead of passing NULL.

This would require unnecessary complication of the code and runtime overhead. 
doprnt is called directly only in few places: esprintf, evxprintf, vmessage, and 
the (rarely-if-ever used) snprintf replacement. If we modified callers to do 
what you're suggesting, we'd need to modify the callers of these functions, and 
their callers too, all the way back to the original ancestor call that specifies 
the format. This would clutter and bloat the code and would add runtime cost to 
all the callers. For example, we'd have to change all calls to the 'error' 
function from something like this:

  if (ret < GNUTLS_E_SUCCESS)
    error ("GnuTLS AEAD cipher %s/%s initialization failed: %s",
           gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));

to something like this:

  if (ret < GNUTLS_E_SUCCESS)
    {
      char const msg[] = "GnuTLS AEAD cipher %s/%s initialization failed: %s";
      error (msg, sizeof msg - 1,
             gnutls_cipher_get_name (gca), desc, emacs_gnutls_strerror (ret));
    }

Of course we could invent a new macro ERROR to package this up, but such a macro 
would still be less efficient than what we have, and worse it would not always 
work, for cases like this:

  if (NILP (tem) || (XBUFFER (tem) != current_buffer))
    error (for_region ? "The mark is not set now, so there is no region"
           : "The mark is not set now");

or like this:

      error (format, string); // in x_check_errors

And of course this could be gotten around as well, but we're now talking about a 
reasonably large amount of code surgery that will hurt code readability and 
runtime performance, all to support a low-level C feature that Emacs does not 
use and won't ever reasonably use.

Instead, we should leave most of the C code alone and just adjust 'doprnt' and 
its very few callers.

> > Drop support for
> > "%S" which is never used and which would cause GCC to warn anyway.
>
> This is an old compatibility feature, I'd rather not drop it.  Who
> knows what code relies on the fact that 'message' and 'format-message'
> support it?

I know because I checked all the code. No Emacs C code uses %S. And none is 
likely to use it in the future because GCC warns about it if you try. (To be 
specific: GCC warns unless you use %S compatibly with its standard C meaning, 
which differs from that of Emacs doprnt - which is yet another compatibility 
minefield if we insist on keeping doprnt's unused %S feature.)