[v6] C++ify gdb/common/environ.c
Commit Message
On Monday, June 19 2017, Pedro Alves wrote:
> On 06/19/2017 05:26 PM, Simon Marchi wrote:
>> On 2017-06-19 17:44, Pedro Alves wrote:
>>> If we take the "always push a NULL on construction" approach, and
>>> we want moved-from gdb_environs to be valid, then yes. Note how this
>>> results in extra heap allocations when e.g., returning a
>>> gdb_environ from functions by value, and makes std::vector<gdb_environ>
>>> much less efficient when it decides it needs to reallocate/move
>>> elements. Representing the empty state with a cleared internal
>>> vector would avoid this.
>>
>> Given the move case, since the goal is to be efficient, then yeah I
>> would agree
>> that it would make sense to make a little bit of efforts to avoid
>> allocating
>> memory for an objects we are almost certainly throwing away.
>>
>> But still, in order to leave environ objects in a valid state after a
>> move and
>> to pedantically comply with the STL spec which says that the vector is
>> left in
>> an unspecified state, shouldn't we do a .clear () on the moved-from
>> vector after
>> the move?
>
> See accepted answer at:
>
> https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty
>
> So the only case where it'd be needed would be in op=, and iff the
> vectors had different allocators, which is not the case here.
> So no, it's not necessary. But I'd be fine with calling it.
>
>>
>>> Note BTW, that we need to be careful with self-move leaving the
>>> *this object in a valid state.
>>
>> Should we just do
>>
>> if (&other == this)
>> return *this;
>
> Might not be necessary if without that the object ends up
> valid anyway. But what you wrote is a safe bet.
So, what do you guys think about the patch below, which applies on top
of the original?
Thanks,
Comments
On 06/19/2017 06:59 PM, Sergio Durigan Junior wrote:
> On Monday, June 19 2017, Pedro Alves wrote:
>
>> On 06/19/2017 05:26 PM, Simon Marchi wrote:
>>> On 2017-06-19 17:44, Pedro Alves wrote:
>>>> If we take the "always push a NULL on construction" approach, and
>>>> we want moved-from gdb_environs to be valid, then yes. Note how this
>>>> results in extra heap allocations when e.g., returning a
>>>> gdb_environ from functions by value, and makes std::vector<gdb_environ>
>>>> much less efficient when it decides it needs to reallocate/move
>>>> elements. Representing the empty state with a cleared internal
>>>> vector would avoid this.
>>>
>>> Given the move case, since the goal is to be efficient, then yeah I
>>> would agree
>>> that it would make sense to make a little bit of efforts to avoid
>>> allocating
>>> memory for an objects we are almost certainly throwing away.
>>>
>>> But still, in order to leave environ objects in a valid state after a
>>> move and
>>> to pedantically comply with the STL spec which says that the vector is
>>> left in
>>> an unspecified state, shouldn't we do a .clear () on the moved-from
>>> vector after
>>> the move?
>>
>> See accepted answer at:
>>
>> https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty
>>
>> So the only case where it'd be needed would be in op=, and iff the
>> vectors had different allocators, which is not the case here.
>> So no, it's not necessary. But I'd be fine with calling it.
>>
>>>
>>>> Note BTW, that we need to be careful with self-move leaving the
>>>> *this object in a valid state.
>>>
>>> Should we just do
>>>
>>> if (&other == this)
>>> return *this;
>>
>> Might not be necessary if without that the object ends up
>> valid anyway. But what you wrote is a safe bet.
>
> So, what do you guys think about the patch below, which applies on top
> of the original?
Missed fixing move ctor?
+ /* Move constructor. */
+ gdb_environ (gdb_environ &&e)
+ : m_environ_vector (std::move (e.m_environ_vector))
+ {}
Thanks,
Pedro Alves
On Monday, June 19 2017, Pedro Alves wrote:
> On 06/19/2017 06:59 PM, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, Pedro Alves wrote:
>>
>>> On 06/19/2017 05:26 PM, Simon Marchi wrote:
>>>> On 2017-06-19 17:44, Pedro Alves wrote:
>>>>> If we take the "always push a NULL on construction" approach, and
>>>>> we want moved-from gdb_environs to be valid, then yes. Note how this
>>>>> results in extra heap allocations when e.g., returning a
>>>>> gdb_environ from functions by value, and makes std::vector<gdb_environ>
>>>>> much less efficient when it decides it needs to reallocate/move
>>>>> elements. Representing the empty state with a cleared internal
>>>>> vector would avoid this.
>>>>
>>>> Given the move case, since the goal is to be efficient, then yeah I
>>>> would agree
>>>> that it would make sense to make a little bit of efforts to avoid
>>>> allocating
>>>> memory for an objects we are almost certainly throwing away.
>>>>
>>>> But still, in order to leave environ objects in a valid state after a
>>>> move and
>>>> to pedantically comply with the STL spec which says that the vector is
>>>> left in
>>>> an unspecified state, shouldn't we do a .clear () on the moved-from
>>>> vector after
>>>> the move?
>>>
>>> See accepted answer at:
>>>
>>> https://stackoverflow.com/questions/17730689/is-a-moved-from-vector-always-empty
>>>
>>> So the only case where it'd be needed would be in op=, and iff the
>>> vectors had different allocators, which is not the case here.
>>> So no, it's not necessary. But I'd be fine with calling it.
>>>
>>>>
>>>>> Note BTW, that we need to be careful with self-move leaving the
>>>>> *this object in a valid state.
>>>>
>>>> Should we just do
>>>>
>>>> if (&other == this)
>>>> return *this;
>>>
>>> Might not be necessary if without that the object ends up
>>> valid anyway. But what you wrote is a safe bet.
>>
>> So, what do you guys think about the patch below, which applies on top
>> of the original?
>
> Missed fixing move ctor?
>
> + /* Move constructor. */
> + gdb_environ (gdb_environ &&e)
> + : m_environ_vector (std::move (e.m_environ_vector))
> + {}
Indeed. Fixed now. I'll submit v7.
Thanks,
On 06/19/2017 07:23 PM, Sergio Durigan Junior wrote:
> On Monday, June 19 2017, Pedro Alves wrote:
>> Missed fixing move ctor?
>>
>> + /* Move constructor. */
>> + gdb_environ (gdb_environ &&e)
>> + : m_environ_vector (std::move (e.m_environ_vector))
>> + {}
>
> Indeed. Fixed now. I'll submit v7.
Make sure to add unit tests for the move ctor. E.g., reuse
the move assign tests.
Thanks,
Pedro Alves
On 06/19/2017 07:36 PM, Pedro Alves wrote:
>
> On 06/19/2017 07:23 PM, Sergio Durigan Junior wrote:
>> On Monday, June 19 2017, Pedro Alves wrote:
>
>>> Missed fixing move ctor?
>>>
>>> + /* Move constructor. */
>>> + gdb_environ (gdb_environ &&e)
>>> + : m_environ_vector (std::move (e.m_environ_vector))
>>> + {}
>>
>> Indeed. Fixed now. I'll submit v7.
>
> Make sure to add unit tests for the move ctor. E.g., reuse
> the move assign tests.
And self-move tests...
Thanks,
Pedro Alves
@@ -25,13 +25,18 @@
gdb_environ &
gdb_environ::operator= (gdb_environ &&e)
{
- clear ();
+ if (&e == this)
+ 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;
}
/* Create a gdb_environ object using the host's environment
variables. */
+
gdb_environ gdb_environ::from_host_environ ()
{
extern char **environ;
@@ -67,9 +67,7 @@ public:
char **envp () const;
private:
- /* A vector containing the environment variables. This is useful
- for when we need to obtain a 'char **' with all the existing
- variables. */
+ /* A vector containing the environment variables. */
std::vector<char *> m_environ_vector;
};
@@ -2151,12 +2151,11 @@ environment_info (char *var, int from_tty)
{
char **envp = current_inferior ()->environment.envp ();
- if (envp != NULL)
- for (int idx = 0; envp[idx] != NULL; ++idx)
- {
- puts_filtered (envp[idx]);
- puts_filtered ("\n");
- }
+ for (int idx = 0; envp[idx] != NULL; ++idx)
+ {
+ puts_filtered (envp[idx]);
+ puts_filtered ("\n");
+ }
}
}
@@ -27,36 +27,58 @@ namespace gdb_environ_tests {
static void
run_tests ()
{
+ /* Set a test environment variable. This will be unset at the end
+ of this function. */
if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
- error ("Could not set environment variable for testing.");
+ error (_("Could not set environment variable for testing."));
gdb_environ env;
+ /* When the vector is initialized, there should always be one NULL
+ element in it. */
SELF_CHECK (env.envp ()[0] == NULL);
+ /* Make sure that there is no other element. */
SELF_CHECK (env.get ("PWD") == NULL);
+
+ /* Check if unset followed by a set in an empty vector works. */
env.set ("PWD", "test");
+ SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0);
+ /* The second element must be NULL. */
+ SELF_CHECK (env.envp ()[1] == NULL);
env.unset ("PWD");
+ SELF_CHECK (env.envp ()[0] == NULL);
+ /* Initialize the environment vector using the host's environ. */
env = gdb_environ::from_host_environ ();
+ /* Our test environment variable should be present at the
+ vector. */
SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
+ /* Set our test variable to another value. */
env.set ("GDB_SELFTEST_ENVIRON", "test");
SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "test") == 0);
+ /* And unset our test variable. The variable still exists in the
+ host's environment, but doesn't exist in our vector. */
env.unset ("GDB_SELFTEST_ENVIRON");
SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
+ /* Re-set the test variable. */
env.set ("GDB_SELFTEST_ENVIRON", "1");
SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
+ /* When we clear our environ vector, there should be only one
+ element on it (NULL), and we shouldn't be able to get our test
+ variable. */
env.clear ();
-
SELF_CHECK (env.envp ()[0] == NULL);
-
SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
+ /* Reinitialize our environ vector using the host environ. We
+ should be able to see one (and only one) instance of the test
+ variable. */
env = gdb_environ::from_host_environ ();
char **envp = env.envp ();
int num_found = 0;
@@ -64,11 +86,14 @@ run_tests ()
for (size_t i = 0; envp[i] != NULL; ++i)
if (strcmp (envp[i], "GDB_SELFTEST_ENVIRON=1") == 0)
++num_found;
-
SELF_CHECK (num_found == 1);
+ /* Get rid of our test variable. */
unsetenv ("GDB_SELFTEST_ENVIRON");
+ /* Test the case when we set a variable A, then set a variable B,
+ then unset A, and make sure that we cannot find A in the environ
+ vector, but can still find B. */
env.set ("GDB_SELFTEST_ENVIRON_1", "aaa");
SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
@@ -78,6 +103,21 @@ run_tests ()
env.unset ("GDB_SELFTEST_ENVIRON_1");
SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON_1") == NULL);
SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
+
+ env.clear ();
+
+ /* Test that after a std::move the moved-from object is left at a
+ valid state (i.e., its only element is NULL). */
+ env.set ("A", "1");
+ SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
+ gdb_environ env2;
+ env2 = std::move (env);
+ SELF_CHECK (env.envp ()[0] == NULL);
+ SELF_CHECK (strcmp (env2.get ("A"), "1") == 0);
+ SELF_CHECK (env2.envp ()[1] == NULL);
+ env.set ("B", "2");
+ SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
+ SELF_CHECK (env.envp ()[1] == NULL);
}
} /* namespace gdb_environ */
} /* namespace selftests */