From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119362 invoked by alias); 19 Oct 2016 21:45:01 -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 119335 invoked by uid 89); 19 Oct 2016 21:45:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:2305, safer, describes, null-terminated X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Oct 2016 21:44:50 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9F2F843A5A; Wed, 19 Oct 2016 21:44:49 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9JLimrb019081; Wed, 19 Oct 2016 17:44:48 -0400 Subject: Re: [PATCH v2 04/31] cli-script.c: Simplify using std::string, eliminate cleanups To: Simon Marchi References: <1476839539-8374-1-git-send-email-palves@redhat.com> <1476839539-8374-5-git-send-email-palves@redhat.com> <72d887888ab5002b79165c861734d6d2@simark.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <0d3ae9d6-85eb-2b78-5e42-cf6167d84cc2@redhat.com> Date: Wed, 19 Oct 2016 21:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <72d887888ab5002b79165c861734d6d2@simark.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00585.txt.bz2 On 10/19/2016 07:25 PM, Simon Marchi wrote: > On 2016-10-18 21:11, Pedro Alves wrote: >> @@ -457,12 +456,13 @@ execute_control_command (struct command_line *cmd) >> switch (cmd->control_type) >> { >> case simple_control: >> - /* A simple command, execute it and return. */ >> - new_line = insert_args (cmd->line); >> - make_cleanup (free_current_contents, &new_line); >> - execute_command (new_line, 0); >> - ret = cmd->control_type; >> - break; >> + { >> + /* A simple command, execute it and return. */ >> + std::string new_line = insert_args (cmd->line); >> + execute_command (&new_line[0], 0); > > The need to use &new_line[0] instead of .c_str() here looks (or smells) > like a code smell to me. See my comment to the string_printf patch. &str[0] is required to return a pointer to modifiable contents. What's not technically defined is assuming that that returns a NULL-terminated buffer. But in practice it does, everywhere. See. The second answer describes it fully: http://stackoverflow.com/questions/347949/how-to-convert-a-stdstring-to-const-char-or-char Writing a NULL to the middle of the string via the char pointer won't change the std::string's size(), but that's not a problem here, because new_line is a temporary and we don't care about the new_line.size() after the execute_command call. > If execute_command needs to modify the string, > it should either make its own copy, or at least document in what ways it > can be modified. And in general, modifying an std::string's underlying > array is probably not good (if I understand the standard correctly, it's > undefined behaviour, though in reality it probably always works). See above. > Do you know of any caller of execute_command that relies on the > modification that execute_command does to the passed string? The main execute_command call coming from the tty. :-) Some commands hack the command line to influence what's actually repeated with . Off hand, I remember list_command: /* If this command is repeated with RET, turn it into the no-arg variant. */ if (from_tty) *arg = 0; > If not, I > think it would be safer to change the arg to a const char * and > duplicate the string at function entry. Thanks, Pedro Alves