[PATCH v2] C++ify gdb/common/environ.c
Sergio Durigan Junior
sergiodj@redhat.com
Tue Apr 18 02:49:00 GMT 2017
Thanks for the review, Simon. I don't know why I didn't receive this
message on my INBOX, so it took a little bit until I saw it on
gdb-patches.
On Saturday, April 15 2017, Simon Marchi wrote:
> On 2017-04-15 14:50, Sergio Durigan Junior wrote:
>> diff --git a/gdb/charset.c b/gdb/charset.c
>> index f55e482..a5ab383 100644
>> --- a/gdb/charset.c
>> +++ b/gdb/charset.c
>> @@ -794,16 +794,14 @@ find_charset_names (void)
>> int err, status;
>> int fail = 1;
>> int flags;
>> - struct gdb_environ *iconv_env;
>> + std::unique_ptr<gdb_environ> iconv_env (new gdb_environ);
>
> Can it be allocated on the stack?
Yeah, it can. It's probably an overkill to use a unique_ptr here
because the object will be short-lived anyway. I'll change that.
>> void
>> -set_in_environ (struct gdb_environ *e, const char *var, const char
>> *value)
>> +gdb_environ::set_in_environ (const char *var, const char *value)
>> {
>> - int i;
>> - int len = strlen (var);
>> - char **vector = e->vector;
>> - char *s;
>> + std::unordered_map<std::string, std::string>::iterator el;
>> + bool needs_update = true;
>>
>> - for (i = 0; (s = vector[i]) != NULL; i++)
>> - if (strncmp (s, var, len) == 0 && s[len] == '=')
>> - break;
>> + el = this->env.find (var);
>>
>> - if (s == 0)
>> + if (el != this->env.end ())
>> {
>> - if (i == e->allocated)
>> + if (el->second.compare (value) == 0)
>
> I think operator== will do what you want.
Good catch, thanks.
> If you did a return here, you wouldn't need the needs_update variable.
> I don't mind though, some people prefer a single return point.
No, you're totally right, I overlooked this detail. Changed to a
return.
>> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
>> index 3ace69e..c630425 100644
>> --- a/gdb/common/environ.h
>> +++ b/gdb/common/environ.h
>> @@ -17,33 +17,55 @@
>> #if !defined (ENVIRON_H)
>> #define ENVIRON_H 1
>>
>> -/* We manipulate environments represented as these structures. */
>> +#include <unordered_map>
>> +#include <vector>
>
> In the interface of gdb_environ, I think you could get rid of the
> "environ" in method names. You can also remove the (void) for methods
> without parameters.
Indeed, the "environ" in the names doesn't make sense anymore.
As for removing (void)... I personally like to make it explicit, but I
understand we're living in other times now (C++11 and all). I'll
remove.
>>
>> -struct gdb_environ
>> - {
>> - /* Number of usable slots allocated in VECTOR.
>> - VECTOR always has one slot not counted here,
>> - to hold the terminating zero. */
>> - int allocated;
>> - /* A vector of slots, ALLOCATED + 1 of them.
>> - The first few slots contain strings "VAR=VALUE"
>> - and the next one contains zero.
>> - Then come some unused slots. */
>> - char **vector;
>> - };
>> +/* Class that represents the environment variables as seeing by the
>
> s/as seeing/as seen/
Fixed.
>> + inferior. */
>>
>> -extern struct gdb_environ *make_environ (void);
>> +class gdb_environ
>> +{
>> +public:
>> + /* Regular constructor and destructor. */
>> + gdb_environ ();
>> + ~gdb_environ ();
>>
>> -extern void free_environ (struct gdb_environ *);
>> + /* Reinitialize the environment stored. This is used when the user
>> + wants to delete all environment variables added by him/her. */
>> + void reinit_environ (void);
>>
>> -extern void init_environ (struct gdb_environ *);
>> + /* Return the value in the environment for the variable VAR. */
>> + char *get_in_environ (const char *var) const;
>
> Looking at the users, I think that this could return const char *. It
> would also be good to mention that the pointer is valid as long as the
> corresponding variable environment is not removed/replaced.
Fair enough. I constified the return type of the 'get' method, and
changed the callers accordingly. I've also added the
> It could also return something based on std::string, but I don't know
> how good it would be:
>
> 1. returning const std::string &: can't return "NULL".
> 2. returning gdb::optional<std::string>: it works but makes a copy of
> the string, maybe it's ok since there isn't a ton of that. It would
> however make the return value usable even after the corresponding
> variable is removed from the env.
> 3. returning gdb::optional<const std::string &>: I don't think it can
> be done.
> 4. returning gdb::optional<std::reference_wrapper<const std::string>>:
> it works, but it gets a bit ugly.
Ahm, I'm not sure. I mean, I know the rationale here (you're trying to
make sure that the return value only exists for as long as the
environment variable is there), but I don't think it's necessary to
overcomplicate things for now. IMHO, a 'const char *' + the updated
comment are enough.
> One thing is sure, since you are constructing some std::string inside
> the methods, you could change the parameters in the interface from
> const char * to const std::string &. With the implicit string
> constructor from const char *, the users won't have to be changed, but
> it will be possible to pass an std::string directly.
I confess this hasn't even crossed my mind. Thanks, I adopted the idea.
>>
>> -extern char *get_in_environ (const struct gdb_environ *, const char
>> *);
>> + /* Store VAR=VALUE in the environment. */
>> + void set_in_environ (const char *var, const char *value);
>>
>> -extern void set_in_environ (struct gdb_environ *, const char *,
>> const char *);
>> + /* Unset VAR in environment. */
>> + void unset_in_environ (const char *var);
>>
>> -extern void unset_in_environ (struct gdb_environ *, const char *);
>> + /* Return the environment vector represented as a 'char **'. */
>> + char **get_environ_char_vector (void) const;
>>
>> -extern char **environ_vector (struct gdb_environ *);
>> + /* Return the environment vector. */
>> + std::vector<char *> get_environ_vector (void) const;
>
> Return a const reference, and change the (sole) user of this method to
> get a reference too?
Heh, I had done that before I got to this part of the e-mail. Thanks.
>> @@ -2159,11 +2160,12 @@ environment_info (char *var, int from_tty)
>> }
>> else
>> {
>> - char **vector = environ_vector (current_inferior
>> ()->environment);
>> + std::vector<char *> vector =
>> + current_inferior ()->environment->get_environ_vector ();
>>
>> - while (*vector)
>> + for (char *&elem : vector)
>
> Is there advantage of doing
>
> for (char *&elem : vector)
>
> vs
>
> for (char *elem : vector)
>
> ?
No, this is just a brain fart. I wrote this code when I was also
dealing with the fork-inferior sharing stuff, and you can see the same
pattern there. Fixed.
>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index b5eb3d1..e48d5cb 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -42,6 +42,9 @@ struct continuation;
>> /* For struct frame_id. */
>> #include "frame.h"
>>
>> +/* For gdb_environ. */
>> +#include "environ.h"
>> +
>
> You can remove the "struct gdb_environ;" above this.
Fixed.
>
>> #include "progspace.h"
>> #include "registry.h"
>>
>> @@ -362,7 +365,7 @@ public:
>>
>> /* Environment to use for running inferior,
>> in format described in environ.h. */
>> - gdb_environ *environment = NULL;
>> + std::unique_ptr<gdb_environ> environment;
>
> Can this be simply
>
> gdb_environ environment;
>
> ?
>
>> @@ -270,7 +270,7 @@ mi_cmd_inferior_tty_show (const char *command,
>> char **argv, int argc)
>> void
>> _initialize_mi_cmd_env (void)
>> {
>> - struct gdb_environ *environment;
>> + std::unique_ptr<gdb_environ> environment (new gdb_environ);
>
> Again, can it be allocated on the stack?
Yes, already done that. Thanks.
I'll send v3 soon.
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
More information about the Gdb-patches
mailing list