From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59448 invoked by alias); 19 Apr 2017 04:56:38 -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 59429 invoked by uid 89); 19 Apr 2017 04:56:37 -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= 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; Wed, 19 Apr 2017 04:56:35 +0000 Received: by simark.ca (Postfix, from userid 33) id 272CD1E165; Wed, 19 Apr 2017 00:56:35 -0400 (EDT) To: Sergio Durigan Junior Subject: Re: [PATCH v3] 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: Wed, 19 Apr 2017 04:56:00 -0000 From: Simon Marchi Cc: GDB Patches In-Reply-To: <20170418030319.12637-1-sergiodj@redhat.com> References: <20170413040455.23996-1-sergiodj@redhat.com> <20170418030319.12637-1-sergiodj@redhat.com> Message-ID: <843eaee444605d981d452a1801a24ebf@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00549.txt.bz2 On 2017-04-17 23:03, Sergio Durigan Junior wrote: > Changes from v2 (mostly from Simon's review): > > - Not using std::unique_ptr for gdb_environ anymore. > > - Got rid of "_environ" suffixes from the names of the methods. > > - Remove (void) from methods without parameters. > > - Constify return of 'get' method. > > - Change 'const char *' for 'const std::string &' on methods' > parameters. > > - Returning a 'const std::vector &' on the 'get_vector' > method. > > - Typos, and other minor nits. > > > > Disclaimer: this patch depends on > to be > fully testable. > > As part of the preparation necessary for my upcoming task, I'd like to > propose that we turn gdb_environ into a class. The approach taken > here is simple: the class gdb_environ contains everything that is > needed to manipulate the environment variables. These variables are > stored in two data structures: an unordered_set, good because lookups > are O(n), and an std::vector, which can be converted to a > 'char **' and passed as argument to functions that need it. It still says O(n) ;) > diff --git a/gdb/common/environ.c b/gdb/common/environ.c > index 3145d01..131835e 100644 > --- a/gdb/common/environ.c > +++ b/gdb/common/environ.c > @@ -18,165 +18,156 @@ > #include "common-defs.h" > #include "environ.h" > #include > - > +#include > > -/* Return a new environment object. */ > +/* See common/environ.h. */ > > -struct gdb_environ * > -make_environ (void) > +void > +gdb_environ::init () > { > - struct gdb_environ *e; > + extern char **environ; > + > + if (environ == NULL) > + return; > > - e = XNEW (struct gdb_environ); > + for (int i = 0; environ[i] != NULL; ++i) > + { > + std::string v = std::string (environ[i]); > + std::size_t pos = v.find ('='); > + std::string var = v.substr (0, pos); > + std::string value = v.substr (pos + 1); > > - e->allocated = 10; > - e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char > *)); > - e->vector[0] = 0; > - return e; > + this->m_environ_map.insert (std::make_pair (var, value)); I see that make_pair has a move/xvalue version: template< class T1, class T2 > std::pair make_pair( T1&& t, T2&& u ); Does that mean that we could/should do this->m_environ_map.insert (std::make_pair (std::move (var), std::move (value))); in order to reuse the resources of the existing strings instead of copying them? > +const char * > +gdb_environ::get (const std::string &var) const > +{ > + try > { > - e->allocated = std::max (i, e->allocated + 10); > - e->vector = (char **) xrealloc ((char *) e->vector, > - (e->allocated + 1) * sizeof (char *)); > + return (char *) this->m_environ_map.at (var).c_str (); You can remove the cast here. I didn't think about it at first, but some unit tests for this class would be nice as well. Thanks, Simon