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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Monday, June 19 2017, Pedro Alves wrote:

> 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):

Yes, I know, I was just correcting the last part of your sentence:

  I didn't spot any check making sure that invariant holds after *an*
  unset.

(emphasis mine)

> 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.

Right, I'll add guiding comments.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/