Re: [PATCH v5] C++ify gdb/common/environ.c

[Pedro Alves <palves@redhat.com>]

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 <palves@redhat.com>
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<char *>::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<char **> (&m_environ_vector[0]);
+  const char *const *e = (m_environ_vector.empty ()
+			  ? empty_envp
+			  : &m_environ_vector[0]);
+  return const_cast<char **> (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<char *> m_environ_vector;
 };
 
-- 
2.5.5