From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id AC52C3870842 for ; Tue, 12 May 2020 17:53:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AC52C3870842 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id F2C0E1ED39; Tue, 12 May 2020 13:53:44 -0400 (EDT) Subject: Re: [PATCH v3 2/6] gdbsupport: Adapt construct_inferior_arguments To: Michael Weghorn , gdb-patches@sourceware.org References: <20200429111638.1327262-1-m.weghorn@posteo.de> <20200512154211.1311364-1-m.weghorn@posteo.de> <20200512154211.1311364-2-m.weghorn@posteo.de> From: Simon Marchi Message-ID: Date: Tue, 12 May 2020 13:53:44 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200512154211.1311364-2-m.weghorn@posteo.de> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 May 2020 17:53:46 -0000 Just some nits. On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote: > diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc > index a7d631f357..cadbd50e9c 100644 > --- a/gdbsupport/common-inferior.cc > +++ b/gdbsupport/common-inferior.cc > @@ -27,15 +27,12 @@ bool startup_with_shell = true; > > /* See common-inferior.h. */ > > -char * > -construct_inferior_arguments (int argc, char **argv) > +std::string > +construct_inferior_arguments (int argc, char * const *argv) > { > - char *result; > + gdb_assert (argc >= 0); > > - /* ARGC should always be at least 1, but we double check this > - here. This is also needed to silence -Werror-stringop > - warnings. */ > - gdb_assert (argc > 0); > + std::string result; > > if (startup_with_shell) > { > @@ -52,48 +49,39 @@ construct_inferior_arguments (int argc, char **argv) > static const char quote = '\''; > #endif > int i; Can you inline this in the for loop while at it? > - int length = 0; > - char *out, *cp; > - > - /* We over-compute the size. It shouldn't matter. */ > - for (i = 0; i < argc; ++i) > - length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0'); > - > - result = (char *) xmalloc (length); > - out = result; > > for (i = 0; i < argc; ++i) > { > if (i > 0) > - *out++ = ' '; > + result += ' '; > > /* Need to handle empty arguments specially. */ > if (argv[i][0] == '\0') > { > - *out++ = quote; > - *out++ = quote; > + result += quote; > + result += quote; > } > else > { > #ifdef __MINGW32__ > - int quoted = 0; > + bool quoted = 0; Replace 0 with false. > > if (strpbrk (argv[i], special)) > { > - quoted = 1; > - *out++ = quote; > + quoted = true; > + result += quote; > } > #endif > - for (cp = argv[i]; *cp; ++cp) > + for (char *cp = argv[i]; *cp; ++cp) > { > if (*cp == '\n') > { > /* A newline cannot be quoted with a backslash (it > just disappears), only by putting it inside > quotes. */ > - *out++ = quote; > - *out++ = '\n'; > - *out++ = quote; > + result += quote; > + result += '\n'; > + result += quote; > } > else > { > @@ -102,24 +90,22 @@ construct_inferior_arguments (int argc, char **argv) > #else > if (strchr (special, *cp) != NULL) > #endif > - *out++ = '\\'; > - *out++ = *cp; > + result += '\\'; > + result += *cp; > } > } > #ifdef __MINGW32__ > if (quoted) > - *out++ = quote; > + result += quote; > #endif > } > } > - *out = '\0'; > } > else > { > /* In this case we can't handle arguments that contain spaces, > tabs, or newlines -- see breakup_args(). */ > int i; Can you also inline this one in the for loops? Simon