This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] C++ify gdb/common/environ.c
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