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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Monday, June 19 2017, Pedro Alves wrote:

> On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
>> +private:
>> +  /* A vector containing the environment variables.  This is useful
>> +     for when we need to obtain a 'char **' with all the existing
>> +     variables.  */
>> +  std::vector<char *> m_environ_vector;
>> +};
>
> This "This is useful" comment doesn't seem to make much
> sense here in isolation.  What exactly is useful, and in comparison
> to what else?  Maybe you're referring to the choice of type of element
> in the vector, say vs a unique_ptr.  Please clarify the comment.  As
> is, it would sound like a comment more fit to the class'es intro
> or to the envp() method.

This is probably a leftover comment from a very early version.  I
removed the part about usefulness.

> On 06/19/2017 05:35 AM, Sergio Durigan Junior wrote:
>>    else
>>      {
>> -      char **vector = environ_vector (current_inferior ()->environment);
>> +      char **envp = current_inferior ()->environment.envp ();
>>  
>> -      while (*vector)
>> -	{
>> -	  puts_filtered (*vector++);
>> -	  puts_filtered ("\n");
>> -	}
>> +      if (envp != NULL)
>
> I suspect this NULL check here was only needed in the previous
> version that mishandled empty environs.  I can't see how it
> makes sense now.  If you still need it, then there's a bug
> elsewhere.

No, it is not needed anymore.  Removed.

>> +	for (int idx = 0; envp[idx] != NULL; ++idx)
>> +	  {
>> +	    puts_filtered (envp[idx]);
>> +	    puts_filtered ("\n");
>> +	  }
>>      }
>
>
>
>> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
>> +    error ("Could not set environment variable for testing.");
>
> Missing _() around error's format string.

Fixed.

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

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/