From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94899 invoked by alias); 31 Jul 2017 20:22:10 -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 94871 invoked by uid 89); 31 Jul 2017 20:22:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 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=scb, opportunity 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; Mon, 31 Jul 2017 20:22:07 +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 v6VKM0vg003900 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 31 Jul 2017 16:22:05 -0400 Received: by simark.ca (Postfix, from userid 112) id 9A9EC1EA01; Mon, 31 Jul 2017 16:22:00 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 01FA41E043; Mon, 31 Jul 2017 16:22:00 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 31 Jul 2017 20:22:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA v2 22/24] Introduce gdb_argv, a class wrapper for buildargv In-Reply-To: <20170725172107.9799-23-tom@tromey.com> References: <20170725172107.9799-1-tom@tromey.com> <20170725172107.9799-23-tom@tromey.com> Message-ID: <5caf2c2b83a56438a2df4cbd3525ef89@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 Mon, 31 Jul 2017 20:22:00 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00457.txt.bz2 On 2017-07-25 19:21, Tom Tromey wrote: > @@ -254,11 +253,10 @@ gdbscm_string_to_argv (SCM string_scm) > return SCM_EOL; > } > > - c_argv = gdb_buildargv (string); > + gdb_argv c_argv (string); > for (i = 0; c_argv[i] != NULL; ++i) Range-based for loop? > diff --git a/gdb/procfs.c b/gdb/procfs.c > index a0d2270..34e1996 100644 > --- a/gdb/procfs.c > +++ b/gdb/procfs.c > @@ -5113,11 +5113,8 @@ procfs_info_proc (struct target_ops *ops, const > char *args, > } > > old_chain = make_cleanup (null_cleanup, 0); > - if (args) > - { > - argv = gdb_buildargv (args); > - make_cleanup_freeargv (argv); > - } > + gdb_argv built_argv (args); > + argv = built_argv.get (); > while (argv != NULL && *argv != NULL) An opportunity to use a range based for ? > @@ -890,7 +888,7 @@ pipe_windows_open (struct serial *scb, const char > *name) > const char *err_msg > = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | > PEX_BINARY_OUTPUT > | PEX_STDERR_TO_PIPE, > - argv[0], argv, NULL, NULL, > + argv[0], argv.get (), NULL, NULL, > &err); > > if (err_msg) > @@ -920,6 +918,7 @@ pipe_windows_open (struct serial *scb, const char > *name) > > scb->state = (void *) ps; > > + argv.release (); This .release() seems to match previous behavior, but that previous behavior looks fishy to me. Nothing seems to take ownership of that argv. The only candidate would be pex_run, but as far as I can see it doesn't. > diff --git a/gdb/stack.c b/gdb/stack.c > index 7f8a51c..9c6d87e 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -1872,13 +1872,14 @@ backtrace_command (char *arg, int from_tty) > int fulltrace_arg = -1, arglen = 0, argc = 0, no_filters = -1; > int user_arg = 0; > > + gdb_argv built_argv; Can this go in the "if" scope? > diff --git a/gdb/utils.c b/gdb/utils.c > index 06f4168..8a7a60f 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -2863,11 +2863,25 @@ ldirname (const char *filename) > return dirname; > } > > +/* See utils.h. */ > + > +void > +gdb_argv::reset (const char *s) > +{ > + char **argv = buildargv (s); > + > + if (s != NULL && argv == NULL) > + malloc_failure (0); > + > + freeargv (m_argv); > + m_argv = argv; > +} > + > /* Call libiberty's buildargv, and return the result. > If buildargv fails due to out-of-memory, call nomem. > Therefore, the returned value is guaranteed to be non-NULL, > unless the parameter itself is NULL. */ > - > + Spurious space added here. The patch LGTM (the .release() in the mingw code can be check later). You can address the other comments at your discretion before pushing. Thanks, Simon