This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v6] C++ify gdb/common/environ.c
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Simon Marchi <simon dot marchi at polymtl dot ca>
- Date: Mon, 19 Jun 2017 12:13:12 -0400
- Subject: Re: [PATCH v6] C++ify gdb/common/environ.c
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9F00080481
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9F00080481
- References: <20170413040455.23996-1-sergiodj@redhat.com> <20170619043531.32394-1-sergiodj@redhat.com> <c13db0c7-ad1f-5ba6-ff95-12004fba9904@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/