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

[Pedro Alves <palves at redhat dot com>]

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.

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.

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




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


Thanks,
Pedro Alves