Re: [PATCH v3] C++ify gdb/common/environ.c

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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<char *> &' on the 'get_vector'
>   method.
>
> - Typos, and other minor nits.
>
>
>
> Disclaimer: this patch depends on
> <https://sourceware.org/ml/gdb-patches/2017-03/msg00551.html> 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<char *>, 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 <algorithm>
> -
> +#include <utility>
>
> -/* 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<V1,V2> 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