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

[Pedro Alves <palves@redhat.com>]

On 06/19/2017 05:13 PM, Sergio Durigan Junior wrote:
> On Monday, June 19 2017, Pedro Alves wrote:

>>> +
>>> +  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.
> 
> This invariant is not supposed to hold after every unset, only after a
> clear or after an unset that removes the only variable in the vector.

... which is exactly the case above.  And for unsets where there are
still elements, the invariant is that the last element is NULL
[which is of course the same invariant].  So maybe we should have a
little function like this (could reuse countargv too):

static size_t
countenvp (const gdb_environ &env)
{
   char **envp = env.envp ();
   size_t count = 0;
   while (envp[count] != NULL)
     count++;
   return count;
}

Used instead of the NULL SELF_CHECKs, like:

  gdb_environ env;

  /* This makes sure that env.envp() is NULL terminated.  */
  SELF_CHECK (countenvp (env) == 0);

  /* ENV is empty, so we shouldn't be able to find any var.  */
  SELF_CHECK (env.get ("PWD") == NULL);

  /* Set a var, and make sure that env.envp() is still NULL
     terminated.  */
  env.set ("PWD", "test");
  SELF_CHECK (countenvp (env) == 1);

  /* Clear the var and make sure that env.envp() is left NULL
     terminated when we remove the last element.  */
  env.unset ("PWD");
  SELF_CHECK (countenvp (env) == 0);

etc.

I find that adding guiding comments like above helps, btw.

Thanks,
Pedro Alves