[v5] C++ify gdb/common/environ.c
Commit Message
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(-)
@@ -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);
}
@@ -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;
};