From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67752 invoked by alias); 19 Jun 2017 14:26:00 -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 67644 invoked by uid 89); 19 Jun 2017 14:25:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Hx-languages-length:1788 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; Mon, 19 Jun 2017 14:25:54 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EABA183F47; Mon, 19 Jun 2017 14:25:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EABA183F47 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com EABA183F47 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 00EB089C61; Mon, 19 Jun 2017 14:25:54 +0000 (UTC) Subject: Re: [PATCH v6] C++ify gdb/common/environ.c To: Sergio Durigan Junior , GDB Patches References: <20170413040455.23996-1-sergiodj@redhat.com> <20170619043531.32394-1-sergiodj@redhat.com> Cc: Simon Marchi From: Pedro Alves Message-ID: Date: Mon, 19 Jun 2017 14:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170619043531.32394-1-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-06/txt/msg00515.txt.bz2 On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote: > +private: > + /* A vector containing the environment variables. This is useful > + for when we need to obtain a 'char **' with all the existing > + variables. */ > + std::vector m_environ_vector; > +}; This "This is useful" comment doesn't seem to make much sense here in isolation. What exactly is useful, and in comparison to what else? Maybe you're referring to the choice of type of element in the vector, say vs a unique_ptr. Please clarify the comment. As is, it would sound like a comment more fit to the class'es intro or to the envp() method. On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote: > else > { > - char **vector = environ_vector (current_inferior ()->environment); > + char **envp = current_inferior ()->environment.envp (); > > - while (*vector) > - { > - puts_filtered (*vector++); > - puts_filtered ("\n"); > - } > + if (envp != NULL) I suspect this NULL check here was only needed in the previous version that mishandled empty environs. I can't see how it makes sense now. If you still need it, then there's a bug elsewhere. > + for (int idx = 0; envp[idx] != NULL; ++idx) > + { > + puts_filtered (envp[idx]); > + puts_filtered ("\n"); > + } > } > + if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0) > + error ("Could not set environment variable for testing."); Missing _() around error's format string. > + > + gdb_environ env; > + > + SELF_CHECK (env.envp ()[0] == NULL); > + > + SELF_CHECK (env.get ("PWD") == NULL); > + env.set ("PWD", "test"); > + env.unset ("PWD"); > + Please add another SELF_CHECK (env.envp ()[0] == NULL); after the unset. I didn't spot any check making sure that invariant holds after an unset. Thanks, Pedro Alves