[v2,01/31] Introduce string_printf

From: Pedro Alves <palves@redhat.com>

  On 10/19/2016 06:17 PM, Simon Marchi wrote:

>> +/* See documentation in common-utils.h.  */
>> +
>> +std::string
>> +string_printf (const char* fmt, ...)
>> +{
>> +  std::string str;
>> +  va_list vp;
>> +
>> +  /* Start by assuming some reasonable size will be sufficient.  */
>> +  str.resize (1024);
>> +
>> +  while (1)
>> +    {
>> +      size_t size;
>> +      int result;
>> +
>> +      va_start (vp, fmt);
>> +      size = str.size ();
>> +      result = vsnprintf (&str[0], size, fmt, vp);
>> +      va_end (vp);
>> +
>> +      str.resize (result);
> 
> I think you have an off-by-one here, which causes an infinite loop if
> the 1024 bytes buffer is not large enough.  vsnprintf returns the size
> needed without the terminating \0, so the resize here should be of
> "result + 1".  To reproduce/test easily, just change your str.resize
> (1024) to something small.

Whoops...  Good catch.  I should have been more careful.  It's the sort
of code that screams for that sort of bug.  :-)  Brown paper bag on me.

resizing to "result + 1" would be incorrect though.  A
std::string's size does not include the terminating \0.
My idea above was to include the size for the \0 in the
initial resize(1024), and then the last resize fixed
it down.  Probably should have added comments.

The version prior to the one I posted had this:

+      /* While only C++11 or later guarantee std::string has contiguous
+        memory, no known C++03 compiler lays memory non-contiguously.
+        This is a _very_ common hack.  */
+      size = str.size () + 1;
+      result = vsnprintf (const_cast<char*> (str.c_str ()), size, fmt, vp);
+      va_end (vp);
+
+      str.resize (result);

And then thinking that people might dislike the hack, I changed
it to consider the \0 as part of the initial string (std::string
can contain embedded NULLs).

But I shouldn't have.  The "hack" is really not a problem:

  https://herbsutter.com/2008/04/07/cringe-not-vectors-are-guaranteed-to-be-contiguous/#comment-483

I.e., "everyone" does it like that.

> I thought the use of a while loop for this a bit strange, but I
> understand it's to avoid duplicating the code.

Yeah.

> 
> I think you should try writing a unit test for this :).

That's a good point.  :-)  I did that now.

Here's a new version.  Passes testing along with the
rest of the series, and, switches to "pre-compute buffer size",
as I mentioned in the reply to Trevor.

From 32670757bd2647c04d02d01849e417c5a07a6cf5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 19 Oct 2016 20:25:23 +0100
Subject: [PATCH] Introduce string_printf

This introduces the string_printf function.  Like asprintf, but
returns a std::string.

gdb/ChangeLog:
yyyy-mm-yy  Pedro Alves  <palves@redhat.com>

	* Makefile.in (COMMON_OBS): Add utils-selftests.o.
	* common/common-utils.c (string_printf): New function.
	* common/common-utils.h: Include <string>.
	(string_printf): Declare.
	* utils-selftests.c: New file.
---
 gdb/Makefile.in           |  2 +-
 gdb/common/common-utils.c | 25 +++++++++++++++++++++++
 gdb/common/common-utils.h |  6 ++++++
 gdb/utils-selftests.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 gdb/utils-selftests.c

[v2,01/31] Introduce string_printf

Commit Message

Patch