From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42869 invoked by alias); 15 Apr 2017 21:22:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42831 invoked by uid 89); 15 Apr 2017 21:22:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=sole, her X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 15 Apr 2017 21:22:11 +0000 Received: by simark.ca (Postfix, from userid 33) id 6F2AF1E4A1; Sat, 15 Apr 2017 17:22:11 -0400 (EDT) To: Sergio Durigan Junior Subject: Re: [PATCH v2] C++ify gdb/common/environ.c X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 15 Apr 2017 21:22:00 -0000 From: Simon Marchi Cc: GDB Patches In-Reply-To: <20170415185053.31827-1-sergiodj@redhat.com> References: <20170413040455.23996-1-sergiodj@redhat.com> <20170415185053.31827-1-sergiodj@redhat.com> Message-ID: <0a8a772222a48b2c16929100d1263566@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00481.txt.bz2 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 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::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 > +#include 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: 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: I don't think it can be done. 4. returning gdb::optional>: 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 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 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 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 environment (new gdb_environ); Again, can it be allocated on the stack? Thanks, Simon