From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101539 invoked by alias); 18 Apr 2017 02:49:22 -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 101002 invoked by uid 89); 18 Apr 2017 02:49:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=brain, 1733, Hx-spam-relays-external:sk:unused-, 1755 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Apr 2017 02:49:19 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CDB8785545; Tue, 18 Apr 2017 02:49:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CDB8785545 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com CDB8785545 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 91901783D0; Tue, 18 Apr 2017 02:49:19 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches Subject: Re: [PATCH v2] C++ify gdb/common/environ.c References: <20170413040455.23996-1-sergiodj@redhat.com> <20170415185053.31827-1-sergiodj@redhat.com> <0a8a772222a48b2c16929100d1263566@polymtl.ca> X-URL: https://sergiodj.net Date: Tue, 18 Apr 2017 02:49:00 -0000 In-Reply-To: <0a8a772222a48b2c16929100d1263566@polymtl.ca> (Simon Marchi's message of "Sat, 15 Apr 2017 17:22:11 -0400") Message-ID: <87o9vuh1ld.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00501.txt.bz2 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 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::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 >> +#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. 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: 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. 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 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 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 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? 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/