From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36891 invoked by alias); 19 Oct 2017 03:11:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 36870 invoked by uid 89); 19 Oct 2017 03:11:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Oct 2017 03:11:12 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v9J3B5Wv001177 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 18 Oct 2017 23:11:10 -0400 Received: by simark.ca (Postfix, from userid 112) id B061C1E541; Wed, 18 Oct 2017 23:11:05 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 3FE211E055; Wed, 18 Oct 2017 23:11:04 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 19 Oct 2017 03:11:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Tom Tromey , Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Introduce string_appendf/string_vappendf (Re: [RFA 4/6] Simple cleanup removals in remote.c) In-Reply-To: References: <20171016030427.21349-1-tom@tromey.com> <20171016030427.21349-5-tom@tromey.com> <07804bc3-a6c5-2c0f-5730-5dd12fccafbe@ericsson.com> <87fuaipzgg.fsf@tromey.com> <97a40149-a30f-b2af-4441-6945b1d29cf1@redhat.com> <87vajesnor.fsf@tromey.com> Message-ID: <83461717578f600a349e7b308c840047@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 19 Oct 2017 03:11:05 +0000 X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00587.txt.bz2 On 2017-10-16 19:34, Pedro Alves wrote: > On 10/17/2017 12:00 AM, Tom Tromey wrote: >>>>>>> "Pedro" == Pedro Alves 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 > 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 > > * 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