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

[Simon Marchi]

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?

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

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.

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

> 
> -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/

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

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.

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.

> 
> -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?

> @@ -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)

?

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

>  #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?

Thanks,

Simon