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

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

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/