Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c)

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-10-16 19:34, Pedro Alves wrote:
> On 10/17/2017 12:00 AM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> 
>> Pedro> This suggests to me that we're missing a string_printf variant
>> Pedro> that appends to a preexisting string:
>> Pedro>   void string_appendf (std::string &dest, const char* fmt, 
>> ...);
>> Pedro> See (untested) patch below.
>> 
>> Seems like a good idea FWIW.
> 
> Alright, here's a version with unit tests, then.
> 
> From 7d51020e1f1f77d9bfc3a4a06be19d1cbb889500 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 16 Oct 2017 23:22:27 +0100
> Subject: [PATCH] Introduce string_appendf/string_vappendf
> 
> string_appendf is like string_printf, but instead of allocating a new
> string, it appends to an existing string.  This allows reusing a
> std::string's memory buffer across several calls, for example.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* common/common-utils.c (string_appendf, string_vappendf): New
> 	functions.
> 	* common/common-utils.h (string_appendf, string_vappendf): New
> 	declarations.
> 	* remote.c (remote_set_syscall_catchpoint): Use string_append.
> 	* unittests/common-utils-selftests.c (string_appendf_func)
> 	(test_appendf_func, string_vappendf_wrapper, string_appendf_tests)
> 	(string_vappendf_tests): New functions.
> 	(_initialize_common_utils_selftests): Register "string_appendf" and
> 	"string_vappendf tests".
> ---
>  gdb/common/common-utils.c              | 44 
> +++++++++++++++++++++++++++++++
>  gdb/common/common-utils.h              |  9 +++++++
>  gdb/unittests/common-utils-selftests.c | 47 
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index d8c546a..942aebb 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -195,6 +195,50 @@ string_vprintf (const char* fmt, va_list args)
>    return str;
>  }
> 
> +
> +/* See documentation in common-utils.h.  */
> +
> +void
> +string_appendf (std::string &str, const char *fmt, ...)
> +{
> +  va_list vp;
> +  int grow_size;
> +
> +  va_start (vp, fmt);
> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
> +  va_end (vp);
> +
> +  size_t curr_size = str.size ();
> +  str.resize (curr_size + grow_size);
> +
> +  /* C++11 and later guarantee std::string uses contiguous memory and
> +     always includes the terminating '\0'.  */
> +  va_start (vp, fmt);
> +  vsprintf (&str[curr_size], fmt, vp);
> +  va_end (vp);
> +}
> +
> +
> +/* See documentation in common-utils.h.  */
> +
> +void
> +string_vappendf (std::string &str, const char *fmt, va_list args)
> +{
> +  va_list vp;
> +  int grow_size;
> +
> +  va_copy (vp, args);
> +  grow_size = vsnprintf (NULL, 0, fmt, vp);
> +  va_end (vp);
> +
> +  size_t curr_size = str.size ();
> +  str.resize (curr_size + grow_size);
> +
> +  /* C++11 and later guarantee std::string uses contiguous memory and
> +     always includes the terminating '\0'.  */
> +  vsprintf (&str[curr_size], fmt, args);
> +}
> +

string_appendf can be implemented using string_vappendf, to reduce 
duplication.  It would basically be like string_vappendf_wrapper is.  In 
the tests, we can probably just test string_appendf then.

Unless there's a good reason for them not sharing code?

>  char *
>  savestring (const char *ptr, size_t len)
>  {
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 19724f9..a32863c 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -67,6 +67,15 @@ std::string string_printf (const char* fmt, ...)
>  std::string string_vprintf (const char* fmt, va_list args)
>    ATTRIBUTE_PRINTF (1, 0);
> 
> +/* Like string_printf, but appends to DEST instead of returning a new
> +   std::string.  */
> +void string_appendf (std::string &dest, const char* fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
> +
> +/* Like string_appendf, but takes a va_list.  */
> +void string_vappendf (std::string &dest, const char* fmt, va_list 
> args)
> +  ATTRIBUTE_PRINTF (2, 0);
> +
>  /* Make a copy of the string at PTR with LEN characters
>     (and add a null character at the end in the copy).
>     Uses malloc to get the space.  Returns the address of the copy.  */
> diff --git a/gdb/unittests/common-utils-selftests.c
> b/gdb/unittests/common-utils-selftests.c
> index cf65513..9825845 100644
> --- a/gdb/unittests/common-utils-selftests.c
> +++ b/gdb/unittests/common-utils-selftests.c
> @@ -76,6 +76,51 @@ string_vprintf_tests ()
>    test_format_func (format);
>  }
> 
> +/* Type of both 'string_appendf' and the 'string_vappendf_wrapper'
> +   function below.  Used to run the same tests against both
> +   string_appendf and string_vappendf.  */
> +typedef void (string_appendf_func) (std::string &str, const char *fmt, 
> ...);
> +
> +static void
> +test_appendf_func (string_appendf_func *func)
> +{
> +  std::string str;
> +
> +  func (str, "%s", "");
> +  SELF_CHECK (str == "");
> +
> +  func (str, "%s", "test");
> +  SELF_CHECK (str == "test");
> +
> +  func (str, "%d", 23);
> +  SELF_CHECK (str == "test23");
> +
> +  func (str, "%s %d %s", "foo", 45, "bar");
> +  SELF_CHECK (str == "test23foo 45 bar");
> +}
> +
> +static void
> +string_vappendf_wrapper (std::string &str, const char *fmt, ...)
> +{
> +  va_list vp;
> +
> +  va_start (vp, fmt);
> +  string_vappendf (str, fmt, vp);
> +  va_end (vp);
> +}
> +
> +static void
> +string_appendf_tests ()
> +{
> +  test_appendf_func (string_appendf);
> +}
> +
> +static void
> +string_vappendf_tests ()
> +{
> +  test_appendf_func (string_vappendf_wrapper);
> +}
> +
>  } /* namespace selftests */
> 
>  void
> @@ -83,4 +128,6 @@ _initialize_common_utils_selftests ()
>  {
>    selftests::register_test ("string_printf", 
> selftests::string_printf_tests);
>    selftests::register_test ("string_vprintf", 
> selftests::string_vprintf_tests);
> +  selftests::register_test ("string_appendf", 
> selftests::string_appendf_tests);
> +  selftests::register_test ("string_vappendf",
> selftests::string_vappendf_tests);

The last line is too long.

>  }

Otherwise, LGTM.

Simon