[v2,01/31] 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>

	* common/common-utils.c (string_printf): New function.
	* common/common-utils.h: Include <string>.
	(string_printf): Declare.
---
 gdb/common/common-utils.c | 30 ++++++++++++++++++++++++++++++
 gdb/common/common-utils.h |  6 ++++++
 2 files changed, 36 insertions(+)
  

> +std::string
> +string_printf (const char* fmt, ...)

std::string is unfortunately kind of large for a return value
its 4 * sizeof (void *), but maybe the simplicity matters more here.

> +{
> +  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;

you could declare these at first use right?

Trev
  

On 10/19/2016 02:53 PM, Trevor Saunders wrote:
>> +std::string
>> +string_printf (const char* fmt, ...)
> 
> std::string is unfortunately kind of large for a return value
> its 4 * sizeof (void *), but maybe the simplicity matters more here.

That shouldn't matter I think?  Beyond a size, ABIs will return via
the stack instead, and, also, no actual copying (in C++ sense) will
take place, because NRVO should apply here.

> 
>> +{
>> +  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;
> 
> you could declare these at first use right?

Yeah, I considered it, and went back and forth, actually!

It'd look like this instead:

  while (1)
    {
      va_start (vp, fmt);
      size_t size = str.size ();
      int result = vsnprintf (&str[0], size, fmt, vp);
      va_end (vp);

      str.resize (result);

      if (result < size)
	break;
    }

Felt odd to me to see the variables declared in the middle of
va_start/va_end, since those kind of form a scope.  I guess I'm
still not used to declaring PODs i the middle of blocks.  If we
don't that, then when a variable is declared in the middle of a
scope it really stands out that something important might be going
on with the ctor.  But maybe that's just me?  I can certainly
change it. 

Also, while writing this, I was also going back and forth between
doing the initial reserve:

 +  str.resize (1024);

and not doing it, and instead call vsnprintf with a NULL buffer
to pre-compute the necessary size.  The downsize is that
you always have to call vsnprintf twice that way.  The upside
is less wasted memory, considering the case of these strings
ending up stored in some structures.  I saw today that
xstrprintf (via vasprintf) uses the 'pre-compute necessary size'
instead of the 'pre-reserve' approach, so I'm leaning toward
doing that too now.

Thanks,
Pedro Alves
  

On 2016-10-18 21:11, Pedro Alves wrote:
> This introduces the string_printf function.  Like asprintf, but
> returns a std::string.
> 
> gdb/ChangeLog:
> yyyy-mm-yy  Pedro Alves  <palves@redhat.com>
> 
> 	* common/common-utils.c (string_printf): New function.
> 	* common/common-utils.h: Include <string>.
> 	(string_printf): Declare.
> ---
>  gdb/common/common-utils.c | 30 ++++++++++++++++++++++++++++++
>  gdb/common/common-utils.h |  6 ++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index 5a346ec..05ba3aa 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -150,6 +150,36 @@ xsnprintf (char *str, size_t size, const char 
> *format, ...)
>    return ret;
>  }
> 
> +/* 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.

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

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

Simon