From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113215 invoked by alias); 20 Jun 2017 14:02:50 -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 113201 invoked by uid 89); 20 Jun 2017 14:02:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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; Tue, 20 Jun 2017 14:02:43 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF513C056827; Tue, 20 Jun 2017 14:02:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BF513C056827 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com BF513C056827 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 BF5027D90A; Tue, 20 Jun 2017 14:02:37 +0000 (UTC) Subject: Re: [PATCH v5] C++ify gdb/common/environ.c To: Simon Marchi , Sergio Durigan Junior References: <20170413040455.23996-1-sergiodj@redhat.com> <20170616222315.12779-1-sergiodj@redhat.com> <4e43c71a2ac4aa229bb262e18dec668c@polymtl.ca> <631a235c-1f70-4435-3a97-d98b2c385f04@redhat.com> Cc: GDB Patches From: Pedro Alves Message-ID: <9db1cfb1-19c4-5166-4f35-acd52839b4d2@redhat.com> Date: Tue, 20 Jun 2017 14:02: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: <631a235c-1f70-4435-3a97-d98b2c385f04@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-06/txt/msg00573.txt.bz2 On 06/19/2017 01:13 PM, Pedro Alves wrote: > On 06/17/2017 09:54 AM, Simon Marchi wrote: > >> I actually preferred the option of adding the NULL element to the vector >> in the gdb_environ constructor, since it allows always having the vector >> in a consistent state. I don't think that avoiding that heap allocation >> is worth the complexity it adds to the code (unless we can prove >> otherwise by memory usage profiling). > > I'm not exactly sure what complexity this is, but I'm not going to > strongly object to always putting in the NULL element, since that's > what we currently do today. This shows we're missing unit test coverage > at least. Now that the patch is in and we have better unit tests, here's what the alternative looks like. IMO, the level of complexity is equivalent. It's a little more complicated on a couple places, and a little simpler on others. >From d3d4aea4ce0ce3a64008b56664feb68635acadb8 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 20 Jun 2017 14:38:48 +0100 Subject: [PATCH] empty environs --- gdb/common/environ.c | 41 ++++++++++++++++++++++++++--------------- gdb/common/environ.h | 21 ++++++--------------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/gdb/common/environ.c b/gdb/common/environ.c index 2d13957..c2d0d95 100644 --- a/gdb/common/environ.c +++ b/gdb/common/environ.c @@ -30,8 +30,6 @@ gdb_environ::operator= (gdb_environ &&e) return *this; m_environ_vector = std::move (e.m_environ_vector); - e.m_environ_vector.clear (); - e.m_environ_vector.push_back (NULL); return *this; } @@ -42,15 +40,12 @@ gdb_environ gdb_environ::from_host_environ () extern char **environ; gdb_environ e; - if (environ == NULL) + if (environ == NULL || environ[0] == NULL) return e; for (int i = 0; environ[i] != NULL; ++i) - { - /* Make sure we add the element before the last (NULL). */ - e.m_environ_vector.insert (e.m_environ_vector.end () - 1, - xstrdup (environ[i])); - } + e.m_environ_vector.push_back (xstrdup (environ[i])); + e.m_environ_vector.push_back (NULL); return e; } @@ -63,8 +58,6 @@ gdb_environ::clear () for (char *v : m_environ_vector) xfree (v); m_environ_vector.clear (); - /* Always add the NULL element. */ - m_environ_vector.push_back (NULL); } /* Helper function to check if STRING contains an environment variable @@ -99,10 +92,18 @@ gdb_environ::get (const char *var) const void gdb_environ::set (const char *var, const char *value) { - /* We have to unset the variable in the vector if it exists. */ + if (m_environ_vector.empty ()) + { + m_environ_vector.push_back (concat (var, "=", value, NULL)); + m_environ_vector.push_back (NULL); + return; + } + + /* Unset the variable in the vector if it exists. */ unset (var); - /* Insert the element before the last one, which is always NULL. */ + /* Insert the element before the last one, which is the NULL + terminator. */ m_environ_vector.insert (m_environ_vector.end () - 1, concat (var, "=", value, NULL)); } @@ -112,10 +113,13 @@ gdb_environ::set (const char *var, const char *value) void gdb_environ::unset (const char *var) { + if (m_environ_vector.empty ()) + return; + size_t len = strlen (var); - /* We iterate until '.cend () - 1' because the last element is - always NULL. */ + /* We iterate until '.end () - 1' because the last element is the + NULL terminator. */ for (std::vector::iterator el = m_environ_vector.begin (); el != m_environ_vector.end () - 1; ++el) @@ -127,10 +131,17 @@ gdb_environ::unset (const char *var) } } +/* An empty envp. This is what the envp() method returns when the + internal vector is empty. */ +static const char *const empty_envp[1] = { NULL }; + /* See common/environ.h. */ char ** gdb_environ::envp () const { - return const_cast (&m_environ_vector[0]); + const char *const *e = (m_environ_vector.empty () + ? empty_envp + : &m_environ_vector[0]); + return const_cast (e); } diff --git a/gdb/common/environ.h b/gdb/common/environ.h index 0bbb191..1a5494b 100644 --- a/gdb/common/environ.h +++ b/gdb/common/environ.h @@ -25,14 +25,7 @@ class gdb_environ { public: - /* Regular constructor and destructor. */ - gdb_environ () - { - /* Make sure that the vector contains at least a NULL element. - If/when we add more variables to it, NULL will always be the - last element. */ - m_environ_vector.push_back (NULL); - } + gdb_environ () = default; ~gdb_environ () { @@ -42,12 +35,7 @@ public: /* Move constructor. */ gdb_environ (gdb_environ &&e) : m_environ_vector (std::move (e.m_environ_vector)) - { - /* Make sure that the moved-from vector is left at a valid - state (only one NULL element). */ - e.m_environ_vector.clear (); - e.m_environ_vector.push_back (NULL); - } + {} /* Move assignment. */ gdb_environ &operator= (gdb_environ &&e); @@ -74,7 +62,10 @@ public: char **envp () const; private: - /* A vector containing the environment variables. */ + /* A vector containing the environment variables. An initially + empty envp is internally represented with an empty vector to + avoid having to push a NULL (which heap-allocates) on default + construction and on moved-from objects. */ std::vector m_environ_vector; }; -- 2.5.5