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 99890386F80F for ; Thu, 7 May 2020 19:49:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 99890386F80F 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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A58FD1E581; Thu, 7 May 2020 15:49:07 -0400 (EDT) Subject: Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases To: Michael Weghorn , Christian Biesinger , gdb-patches References: <20200429111638.1327262-1-m.weghorn@posteo.de> <20200429111638.1327262-3-m.weghorn@posteo.de> From: Simon Marchi Message-ID: Date: Thu, 7 May 2020 15:49:06 -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: Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.9 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: Thu, 07 May 2020 19:49:18 -0000 Hi Michael, I think the patch looks good, there are just a few nits to fix, and a suggestion. > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 8f7482347c..3ac64b3164 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -151,12 +151,9 @@ get_inferior_args (void) > { > if (current_inferior ()->argc != 0) > { > - char *n; > - > - n = construct_inferior_arguments (current_inferior ()->argc, > + std::string n = construct_inferior_arguments (current_inferior ()->argc, > current_inferior ()->argv); Align the last line with the parenthesis, like it was before the change. > - set_inferior_args (n); > - xfree (n); > + set_inferior_args (n.c_str()); Space before parenthesis. > } > > if (current_inferior ()->args == NULL) > diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc > index 71b9a11e02..3f117d5ef0 100644 > --- a/gdbsupport/common-inferior.cc > +++ b/gdbsupport/common-inferior.cc > @@ -28,15 +28,15 @@ bool startup_with_shell = true; > /* Compute command-line string given argument vector. This does the > same shell processing as fork_inferior. */ > > -char * > -construct_inferior_arguments (int argc, char **argv) > +std::string > +construct_inferior_arguments (int argc, char * const *argv) > { > - char *result; > + gdb_assert (argc >= 0); > + if (argc == 0 || argv[0] == NULL) { > + return ""; > + } Omit the braces when there is just one single line statement. When there are braces, places them like this: if (condition) { stmt1; stmt2; } Did you actually encounter the case (argc > 0 && argv[0] == NULL)? In other words, does it really constitute a valid input to this function? > > - /* 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); > + char *result; > > if (startup_with_shell) > { > @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv) > } > } > > - return result; > + std::string str_result(result); Space before parenthesis. If we are going to return a string anyway, I'd vote for building the result into an std::string directly. You can remove the part that tries to guess the length of the string, and use the appropriate std::string methods / operators to append to it. If you do this, I'm thinking that the `argc == 0` check at the beginning of the function might be unnecessary, as the code could handle it naturally: all the loops will be skipped and we'll return an empty string. Thanks, Simon