[RFA 0/5] Some random C++-ification

[RFA 0/5] Some random C++-ification

[Tom Tromey]

I was feeling inspired by Pedro's Cauldron slides, so I found a few
random spots that could be converted from cleanups to self-managing
data structures from libstdc++ -- in these cases, std::string and
std::vector.

I saw a note in one of the C++ conversion documents about perhaps not
using std::vector, since GCC did not.  However, I think often GCC's
uses are unusual, and I don't think there is any reason to avoid
std::vector in (most of) gdb.

This should probably not go in until after Pedro's "new" patch.

Built and regtested on x86-64 Fedora 24.

A few reflections on these changes:

* First, I think it's a nice improvement.  The examples here aren't so
  dramatic, but if you dig a bit it's easy to find cases where the
  cleanup logic is complicated; and this approach eventually lets one
  delegate all that work to the compiler.

* Speaking of, I have a patch to convert uses of the ensure_python_env
  to use RAII.  However, I don't think this is ready to go in --
  because the cleanups installed by this are order-sensitive with
  respect to other cleanups that might be created in the various
  Python-calling functions.  My belief is that cleanups have to all be
  run before any destructors, so any ordering issues are a subtlety
  that, in the short term, will have to be accounted for in code
  review.

* It wasn't actually clear to me that this kind of change is
  desirable.

* I was unclear on the coding style to use so I just used the gdb C
  style.  Maybe there are some spaces that shouldn't be there now.

Tom

[RFA 3/5] Use std::string, std::vector in rust-lang.c

[Tom Tromey]

This patche changes some spots in rust-lang.c to use std::string or
std::vector, removing some cleanups.

2016-09-22  Tom Tromey  <tom@tromey.com>

	* rust-lang.c: Include <string> and <vector>.
	(rust_evaluate_funcall): Use std::vector, std::string.
	(rust_evaluate_subexp): Use std::string.
	(rust_lookup_symbol_nonlocal): Use std::string.
---
 gdb/ChangeLog   |  7 +++++++
 gdb/rust-lang.c | 38 ++++++++++++++++----------------------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2029bcb..670406d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2016-09-22  Tom Tromey  <tom@tromey.com>
 
+	* rust-lang.c: Include <string> and <vector>.
+	(rust_evaluate_funcall): Use std::vector, std::string.
+	(rust_evaluate_subexp): Use std::string.
+	(rust_lookup_symbol_nonlocal): Use std::string.
+
+2016-09-22  Tom Tromey  <tom@tromey.com>
+
 	* cp-namespace.c: Include <string>.
 	(cp_search_static_and_baseclasses)
 	(cp_lookup_symbol_imports_or_template, find_symbol_in_baseclass):
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 77f7428..b9acfcf 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -32,6 +32,8 @@
 #include "rust-lang.h"
 #include "valprint.h"
 #include "varobj.h"
+#include <string>
+#include <vector>
 
 extern initialize_file_ftype _initialize_rust_language;
 
@@ -1154,10 +1156,9 @@ rust_evaluate_funcall (struct expression *exp, int *pos, enum noside noside)
   int i;
   int num_args = exp->elts[*pos + 1].longconst;
   const char *method;
-  char *name;
+  std::string name;
   struct value *function, *result, *arg0;
-  struct value **args;
-  struct cleanup *cleanup;
+  std::vector<struct value *> args (num_args + 1);
   struct type *type, *fn_type;
   const struct block *block;
   struct block_symbol sym;
@@ -1183,8 +1184,6 @@ rust_evaluate_funcall (struct expression *exp, int *pos, enum noside noside)
       return arg0;
     }
 
-  args = XNEWVEC (struct value *, num_args + 1);
-  cleanup = make_cleanup (xfree, args);
   args[0] = arg0;
 
   /* We don't yet implement real Deref semantics.  */
@@ -1200,17 +1199,16 @@ rust_evaluate_funcall (struct expression *exp, int *pos, enum noside noside)
   if (TYPE_TAG_NAME (type) == NULL)
     error (_("Method call on nameless type"));
 
-  name = concat (TYPE_TAG_NAME (type), "::", method, (char *) NULL);
-  make_cleanup (xfree, name);
+  name = std::string (TYPE_TAG_NAME (type)) + "::" + method;
 
   block = get_selected_block (0);
-  sym = lookup_symbol (name, block, VAR_DOMAIN, NULL);
+  sym = lookup_symbol (name.c_str (), block, VAR_DOMAIN, NULL);
   if (sym.symbol == NULL)
-    error (_("Could not find function named '%s'"), name);
+    error (_("Could not find function named '%s'"), name.c_str ());
 
   fn_type = SYMBOL_TYPE (sym.symbol);
   if (TYPE_NFIELDS (fn_type) == 0)
-    error (_("Function '%s' takes no arguments"), name);
+    error (_("Function '%s' takes no arguments"), name.c_str ());
 
   if (TYPE_CODE (TYPE_FIELD_TYPE (fn_type, 0)) == TYPE_CODE_PTR)
     args[0] = value_addr (args[0]);
@@ -1223,8 +1221,7 @@ rust_evaluate_funcall (struct expression *exp, int *pos, enum noside noside)
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     result = value_zero (TYPE_TARGET_TYPE (fn_type), not_lval);
   else
-    result = call_function_by_hand (function, num_args + 1, args);
-  do_cleanups (cleanup);
+    result = call_function_by_hand (function, num_args + 1, args.data ());
   return result;
 }
 
@@ -1601,14 +1598,11 @@ rust_evaluate_subexp (struct type *expect_type, struct expression *exp,
 	  {
 	    CORE_ADDR addr;
 	    int i;
-	    struct value **eltvec = XNEWVEC (struct value *, copies);
-	    struct cleanup *cleanup = make_cleanup (xfree, eltvec);
+	    std::vector<struct value *> eltvec (copies);
 
 	    for (i = 0; i < copies; ++i)
 	      eltvec[i] = elt;
-	    result = value_array (0, copies - 1, eltvec);
-
-	    do_cleanups (cleanup);
+	    result = value_array (0, copies - 1, eltvec.data ());
 	  }
 	else
 	  {
@@ -2036,14 +2030,14 @@ rust_lookup_symbol_nonlocal (const struct language_defn *langdef,
 
       if (scope[0] != '\0')
 	{
-	  char *scopedname = concat (scope, "::", name, (char *) NULL);
-	  struct cleanup *cleanup = make_cleanup (xfree, scopedname);
+	  std::string scopedname;
+
+	  scopedname = std::string (scope) + "::" + name;
 
-	  result = lookup_symbol_in_static_block (scopedname, block,
+	  result = lookup_symbol_in_static_block (scopedname.c_str (), block,
 						  domain);
 	  if (result.symbol == NULL)
-	    result = lookup_global_symbol (scopedname, block, domain);
-	  do_cleanups (cleanup);
+	    result = lookup_global_symbol (scopedname.c_str (), block, domain);
 	}
     }
   return result;
-- 
2.7.4

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Pedro Alves]

On 09/22/2016 06:50 PM, Tom Tromey wrote:
>    int num_args = exp->elts[*pos + 1].longconst;
>    const char *method;
> -  char *name;
> +  std::string name;

While we required declaring variables at the top of the
scope in C, in C++ it no longer makes sense.  Particularly
if the variable's type as a constructor.

So I think we should move the variable declaration to the
initialization line, to avoid default constructing the variable
and then resetting it afterwards, as the compiler may
not be smart enough to elide that.

>  
> -  name = concat (TYPE_TAG_NAME (type), "::", method, (char *) NULL);
> -  make_cleanup (xfree, name);
> +  name = std::string (TYPE_TAG_NAME (type)) + "::" + method;
>  


> -	  char *scopedname = concat (scope, "::", name, (char *) NULL);
> -	  struct cleanup *cleanup = make_cleanup (xfree, scopedname);
> +	  std::string scopedname;
> +
> +	  scopedname = std::string (scope) + "::" + name;

Here too.  Throughout the series, really.

Thanks,
Pedro Alves

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> So I think we should move the variable declaration to the
Pedro> initialization line, to avoid default constructing the variable
Pedro> and then resetting it afterwards, as the compiler may
Pedro> not be smart enough to elide that.

I think so too -- I had avoided this on account of earlier objections to
this style.

Tom

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Pedro Alves]

On 09/22/2016 08:15 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> So I think we should move the variable declaration to the
> Pedro> initialization line, to avoid default constructing the variable
> Pedro> and then resetting it afterwards, as the compiler may
> Pedro> not be smart enough to elide that.
> 
> I think so too -- I had avoided this on account of earlier objections to
> this style.

In C, it'd require C99, while we were C89.  Other than that,
it was just a coding convention.  

But we now have technical reasons for not putting variables
at top of the scope.  There's the efficiency reason, and then
some types may not even have a default constructor, and the
arguments that'd need to be passed to the constructor
might not have been computed at the top of the scope, making
it impossible to declare the variable at the top, unless we'd
open an ugly new scope...

So I think there's no ground for objection.

Thanks,
Pedro Alves

RE: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : jeudi 22 septembre 2016 21:25
> À : Tom Tromey
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c
> 
> On 09/22/2016 08:15 PM, Tom Tromey wrote:
> >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> >
> > Pedro> So I think we should move the variable declaration to the
> > Pedro> initialization line, to avoid default constructing the
> variable
> > Pedro> and then resetting it afterwards, as the compiler may
> > Pedro> not be smart enough to elide that.
> >
> > I think so too -- I had avoided this on account of earlier objections
> to
> > this style.
> 
> In C, it'd require C99, while we were C89.  Other than that,
> it was just a coding convention.
> 
> But we now have technical reasons for not putting variables
> at top of the scope.  There's the efficiency reason, and then
> some types may not even have a default constructor, and the
> arguments that'd need to be passed to the constructor
> might not have been computed at the top of the scope, making
> it impossible to declare the variable at the top, unless we'd
> open an ugly new scope...

  Just out of curiosity:
how are such variables treated by gcc regarding debug information?

  What is the scope of definition of those variables?
And is it correctly handled by current GDB?

  IIRC (but my knowledge about C++ if far
worse than for plain C.. which itself I only learned to be able to
contribute to GDB...) the reason why we can remove the cleanups is
that the compiler will insert auto--magically
the destructors of those local variables at the end of
their respective scope, is this correct? 
 
> So I think there's no ground for objection.

  Not that I want to raise an objection,
I just wonder if debugging GDB by itself will remain as easy as it was!

Pierre Muller
pascal language support maintainer...

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Pedro Alves]

On 09/22/2016 08:35 PM, Pierre Muller wrote:

>   Just out of curiosity:
> how are such variables treated by gcc regarding debug information?
> 
>   What is the scope of definition of those variables?
> And is it correctly handled by current GDB?

On my system, they look like as if declared at the top of
the scope.  Up until they're initialized, they'll have garbage
contents, and that's what gdb will show.  But that's no different from
actually declaring a variable at the top of the scope and only
initializing it after.  It's not a big deal.

There's a GNU extension that I think is meant to make it possible
for the debugger to treat uninitialized variables
specially (DW_OP_GNU_uninit), though I don't think we do
much with it.  Not sure gcc emits it, even.

> 
>   IIRC (but my knowledge about C++ if far
> worse than for plain C.. which itself I only learned to be able to
> contribute to GDB...) the reason why we can remove the cleanups is
> that the compiler will insert auto--magically
> the destructors of those local variables at the end of
> their respective scope, is this correct? 

Correct.

>  
>> So I think there's no ground for objection.
> 
>   Not that I want to raise an objection,
> I just wonder if debugging GDB by itself will remain as easy as it was!

Hope so too...

Thanks,
Pedro Alves

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> There's a GNU extension that I think is meant to make it possible
Pedro> for the debugger to treat uninitialized variables
Pedro> specially (DW_OP_GNU_uninit), though I don't think we do
Pedro> much with it.

There were never any docs for it :(
I think it probably should have been a piece-terminating operation but
it was never clear if this was how it was actually supposed to work.
I stuck a comment in dwarf_expr_require_composition to this effect back
in the day.

Pedro> Not sure gcc emits it, even.

It does, but I don't know under what conditions.
There's code in dwarf2out.c like:

  if (initialized == VAR_INIT_STATUS_UNINITIALIZED)
    add_loc_descr (&result, new_loc_descr (DW_OP_GNU_uninit, 0, 0));

I don't know when that happens though.

Perhaps this could be cleaned up a bit, say by making DW_OP_GNU_uninit a
piece terminator (I forget the official term here) and by changing gcc
to emit a location list that indicates that the object is uninitialized
until after the constructor is run.  It might be a little friendlier to
users, dunno.

Tom

Re: [RFA 3/5] Use std::string, std::vector in rust-lang.c

[Pedro Alves]

On 09/23/2016 04:49 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> There's a GNU extension that I think is meant to make it possible
> Pedro> for the debugger to treat uninitialized variables
> Pedro> specially (DW_OP_GNU_uninit), though I don't think we do
> Pedro> much with it.
> 
> There were never any docs for it :(

I think all there is the original gcc and gdb patches:

  https://gcc.gnu.org/ml/gcc-patches/2007-05/msg00068.html

I remember seeing these back then, but I was still a gdb noob then.
I don't recall the history of the feature after that.  From a quick look,
it kind of looks like some pieces of the patches are in the respective
trees, but not all hunks.  Maybe they got in, and then mostly reverted.

Thanks,
Pedro Alves

> I think it probably should have been a piece-terminating operation but
> it was never clear if this was how it was actually supposed to work.
> I stuck a comment in dwarf_expr_require_composition to this effect back
> in the day.
> 
> Pedro> Not sure gcc emits it, even.
> 
> It does, but I don't know under what conditions.
> There's code in dwarf2out.c like:
> 
>   if (initialized == VAR_INIT_STATUS_UNINITIALIZED)
>     add_loc_descr (&result, new_loc_descr (DW_OP_GNU_uninit, 0, 0));
> 
> I don't know when that happens though.
> 
> Perhaps this could be cleaned up a bit, say by making DW_OP_GNU_uninit a
> piece terminator (I forget the official term here) and by changing gcc
> to emit a location list that indicates that the object is uninitialized
> until after the constructor is run.  It might be a little friendlier to
> users, dunno.
> 
> Tom
> 

[RFA 1/5] Use std::string in break-catch-sig.c

[Tom Tromey]

This changes one spot in break-catch-sig.c to use std::string,
removing some cleanups.

2016-09-22  Tom Tromey  <tom@tromey.com>

	* break-catch-sig.c: Include <string>.
	(signal_catchpoint_print_one): Use std::string.
---
 gdb/ChangeLog         |  5 +++++
 gdb/break-catch-sig.c | 17 ++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ea14f04..f9924e5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2016-09-22  Tom Tromey  <tom@tromey.com>
+
+	* break-catch-sig.c: Include <string>.
+	(signal_catchpoint_print_one): Use std::string.
+
 2016-09-22  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
 
 	* rs6000-tdep.c (ppc_process_record_op31): Fix
diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 06ee44c..296f900 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -28,7 +28,8 @@
 #include "valprint.h"
 #include "cli/cli-utils.h"
 #include "completer.h"
-#include "gdb_obstack.h"
+
+#include <string>
 
 #define INTERNAL_SIGNAL(x) ((x) == GDB_SIGNAL_TRAP || (x) == GDB_SIGNAL_INT)
 
@@ -265,11 +266,7 @@ signal_catchpoint_print_one (struct breakpoint *b,
     {
       int i;
       gdb_signal_type iter;
-      struct obstack text;
-      struct cleanup *cleanup;
-
-      obstack_init (&text);
-      cleanup = make_cleanup_obstack_free (&text);
+      std::string text;
 
       for (i = 0;
            VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
@@ -278,12 +275,10 @@ signal_catchpoint_print_one (struct breakpoint *b,
 	  const char *name = signal_to_name_or_int (iter);
 
 	  if (i > 0)
-	    obstack_grow (&text, " ", 1);
-	  obstack_grow (&text, name, strlen (name));
+	    text += " ";
+	  text += name;
         }
-      obstack_grow (&text, "", 1);
-      ui_out_field_string (uiout, "what", (const char *) obstack_base (&text));
-      do_cleanups (cleanup);
+      ui_out_field_string (uiout, "what", text.c_str ());
     }
   else
     ui_out_field_string (uiout, "what",
-- 
2.7.4

Re: [RFA 1/5] Use std::string in break-catch-sig.c

[Yao Qi]

On Thu, Sep 22, 2016 at 6:50 PM, Tom Tromey <tom@tromey.com> wrote:
> This changes one spot in break-catch-sig.c to use std::string,
> removing some cleanups.
>
> 2016-09-22  Tom Tromey  <tom@tromey.com>
>
>         * break-catch-sig.c: Include <string>.
>         (signal_catchpoint_print_one): Use std::string.

Patch is good to me.

-- 
Yao (齐尧)

[RFA 4/5] Use std::vector in objfiles.c

[Tom Tromey]

This patch changes a spot in objfiles.c to use a std::vector, removing
a cleanup.

2016-09-22  Tom Tromey  <tom@tromey.com>

	* objfiles.c: Include <vector>.
	(objfile_relocate): Use std::vector.
---
 gdb/ChangeLog  |  5 +++++
 gdb/objfiles.c | 14 +++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 670406d..92e79c3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-09-22  Tom Tromey  <tom@tromey.com>
 
+	* objfiles.c: Include <vector>.
+	(objfile_relocate): Use std::vector.
+
+2016-09-22  Tom Tromey  <tom@tromey.com>
+
 	* rust-lang.c: Include <string> and <vector>.
 	(rust_evaluate_funcall): Use std::vector, std::string.
 	(rust_evaluate_subexp): Use std::string.
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index f022d10..bf594a1 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -53,6 +53,8 @@
 #include "gdb_bfd.h"
 #include "btrace.h"
 
+#include <vector>
+
 /* Keep a registry of per-objfile data-pointers required by other GDB
    modules.  */
 
@@ -943,7 +945,7 @@ objfile_relocate (struct objfile *objfile,
        debug_objfile = objfile_separate_debug_iterate (objfile, debug_objfile))
     {
       struct section_addr_info *objfile_addrs;
-      struct section_offsets *new_debug_offsets;
+      std::vector<struct section_offsets> new_debug_offsets;
       struct cleanup *my_cleanups;
 
       objfile_addrs = build_section_addr_info_from_objfile (objfile);
@@ -956,15 +958,13 @@ objfile_relocate (struct objfile *objfile,
 
       gdb_assert (debug_objfile->num_sections
 		  == gdb_bfd_count_sections (debug_objfile->obfd));
-      new_debug_offsets = 
-	((struct section_offsets *)
-	 xmalloc (SIZEOF_N_SECTION_OFFSETS (debug_objfile->num_sections)));
-      make_cleanup (xfree, new_debug_offsets);
-      relative_addr_info_to_section_offsets (new_debug_offsets,
+      new_debug_offsets
+	.reserve (SIZEOF_N_SECTION_OFFSETS (debug_objfile->num_sections));
+      relative_addr_info_to_section_offsets (new_debug_offsets.data (),
 					     debug_objfile->num_sections,
 					     objfile_addrs);
 
-      changed |= objfile_relocate1 (debug_objfile, new_debug_offsets);
+      changed |= objfile_relocate1 (debug_objfile, new_debug_offsets.data ());
 
       do_cleanups (my_cleanups);
     }
-- 
2.7.4

[RFA 2/5] Use std::string in cp-namespace.c

[Tom Tromey]

This changes a few spots in cp-namespace.c to use std::string,
removing some cleanups.

2016-09-22  Tom Tromey  <tom@tromey.com>

	* cp-namespace.c: Include <string>.
	(cp_search_static_and_baseclasses)
	(cp_lookup_symbol_imports_or_template, find_symbol_in_baseclass):
	Use std::string.
---
 gdb/ChangeLog      |  7 +++++++
 gdb/cp-namespace.c | 51 +++++++++++++++++----------------------------------
 2 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f9924e5..2029bcb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2016-09-22  Tom Tromey  <tom@tromey.com>
 
+	* cp-namespace.c: Include <string>.
+	(cp_search_static_and_baseclasses)
+	(cp_lookup_symbol_imports_or_template, find_symbol_in_baseclass):
+	Use std::string.
+
+2016-09-22  Tom Tromey  <tom@tromey.com>
+
 	* break-catch-sig.c: Include <string>.
 	(signal_catchpoint_print_one): Use std::string.
 
diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 0f1b452..1df59ae 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -32,6 +32,7 @@
 #include "buildsym.h"
 #include "language.h"
 #include "namespace.h"
+#include <string>
 
 static struct block_symbol
   cp_lookup_nested_symbol_1 (struct type *container_type,
@@ -246,8 +247,7 @@ cp_search_static_and_baseclasses (const char *name,
 				  int is_in_anonymous)
 {
   struct block_symbol sym;
-  char *klass, *nested;
-  struct cleanup *cleanup;
+  std::string klass, nested;
   struct block_symbol klass_sym;
   struct type *klass_type;
 
@@ -258,36 +258,28 @@ cp_search_static_and_baseclasses (const char *name,
   /* Find the name of the class and the name of the method, variable, etc.  */
 
   /* The class name is everything up to and including PREFIX_LEN.  */
-  klass = savestring (name, prefix_len);
+  klass = std::string (name, prefix_len);
 
   /* The rest of the name is everything else past the initial scope
      operator.  */
-  nested = xstrdup (name + prefix_len + 2);
-
-  /* Add cleanups to free memory for these strings.  */
-  cleanup = make_cleanup (xfree, klass);
-  make_cleanup (xfree, nested);
+  nested = name + prefix_len + 2;
 
   /* Lookup a class named KLASS.  If none is found, there is nothing
      more that can be done.  KLASS could be a namespace, so always look
      in VAR_DOMAIN.  This works for classes too because of
      symbol_matches_domain (which should be replaced with something else,
      but it's what we have today).  */
-  klass_sym = lookup_global_symbol (klass, block, VAR_DOMAIN);
+  klass_sym = lookup_global_symbol (klass.c_str (), block, VAR_DOMAIN);
   if (klass_sym.symbol == NULL)
-    {
-      do_cleanups (cleanup);
-      return null_block_symbol;
-    }
+    return null_block_symbol;
   klass_type = SYMBOL_TYPE (klass_sym.symbol);
 
   /* Look for a symbol named NESTED in this class.
      The caller is assumed to have already have done a basic lookup of NAME.
      So we pass zero for BASIC_LOOKUP to cp_lookup_nested_symbol_1 here.  */
-  sym = cp_lookup_nested_symbol_1 (klass_type, nested, name, block, domain,
-				   0, is_in_anonymous);
+  sym = cp_lookup_nested_symbol_1 (klass_type, nested.c_str (), name,
+				   block, domain, 0, is_in_anonymous);
 
-  do_cleanups (cleanup);
   return sym;
 }
 
@@ -567,8 +559,7 @@ cp_lookup_symbol_imports_or_template (const char *scope,
       if (SYMBOL_NATURAL_NAME (function))
 	{
 	  struct type *context;
-	  char *name_copy = xstrdup (SYMBOL_NATURAL_NAME (function));
-	  struct cleanup *cleanups = make_cleanup (xfree, name_copy);
+	  std::string name_copy = SYMBOL_NATURAL_NAME (function);
 	  const struct language_defn *lang = language_def (language_cplus);
 	  struct gdbarch *arch = symbol_arch (function);
 	  const struct block *parent = BLOCK_SUPERBLOCK (block);
@@ -576,15 +567,16 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 
 	  while (1)
 	    {
-	      unsigned int prefix_len = cp_entire_prefix_len (name_copy);
+	      unsigned int prefix_len
+		= cp_entire_prefix_len (name_copy.c_str ());
 
 	      if (prefix_len == 0)
 		context = NULL;
 	      else
 		{
-		  name_copy[prefix_len] = '\0';
+		  name_copy.erase (prefix_len, name_copy.size ());
 		  context = lookup_typename (lang, arch,
-					     name_copy,
+					     name_copy.c_str (),
 					     parent, 1);
 		}
 
@@ -597,7 +589,6 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 				      TYPE_TEMPLATE_ARGUMENTS (context));
 	      if (sym != NULL)
 		{
-		  do_cleanups (cleanups);
 		  if (symbol_lookup_debug)
 		    {
 		      fprintf_unfiltered
@@ -608,8 +599,6 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 		  return (struct block_symbol) {sym, parent};
 		}
 	    }
-
-	  do_cleanups (cleanups);
 	}
     }
 
@@ -832,34 +821,28 @@ find_symbol_in_baseclass (struct type *parent_type, const char *name,
 {
   int i;
   struct block_symbol sym;
-  struct cleanup *cleanup;
-  char *concatenated_name;
 
   sym.symbol = NULL;
   sym.block = NULL;
-  concatenated_name = NULL;
-  cleanup = make_cleanup (free_current_contents, &concatenated_name);
 
   for (i = 0; i < TYPE_N_BASECLASSES (parent_type); ++i)
     {
-      size_t len;
       struct type *base_type = TYPE_BASECLASS (parent_type, i);
       const char *base_name = TYPE_BASECLASS_NAME (parent_type, i);
+      std::string concatenated_name;
 
       if (base_name == NULL)
 	continue;
 
-      len = strlen (base_name) + 2 + strlen (name) + 1;
-      concatenated_name = (char *) xrealloc (concatenated_name, len);
-      xsnprintf (concatenated_name, len, "%s::%s", base_name, name);
+      concatenated_name = std::string (base_name) + "::" + name;
 
-      sym = cp_lookup_nested_symbol_1 (base_type, name, concatenated_name,
+      sym = cp_lookup_nested_symbol_1 (base_type, name,
+				       concatenated_name.c_str (),
 				       block, domain, 1, is_in_anonymous);
       if (sym.symbol != NULL)
 	break;
     }
 
-  do_cleanups (cleanup);
   return sym;
 }
 
-- 
2.7.4

Re: [RFA 2/5] Use std::string in cp-namespace.c

[Pedro Alves]

On 09/22/2016 06:50 PM, Tom Tromey wrote:
> -		  name_copy[prefix_len] = '\0';
> +		  name_copy.erase (prefix_len, name_copy.size ());

Passing name_copy.size () here looks odd, since that parameter is
how many bytes to erase.  Just don't pass a second parameter,
which defaults to std::string::npos?

>  		  context = lookup_typename (lang, arch,
> -					     name_copy,
> +					     name_copy.c_str (),
>  					     parent, 1);
>  		}
>  


-- 
Thanks,
Pedro Alves

Re: [RFA 2/5] Use std::string in cp-namespace.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 09/22/2016 06:50 PM, Tom Tromey wrote:
>> -		  name_copy[prefix_len] = '\0';
>> +		  name_copy.erase (prefix_len, name_copy.size ());

Pedro> Passing name_copy.size () here looks odd, since that parameter is
Pedro> how many bytes to erase.  Just don't pass a second parameter,
Pedro> which defaults to std::string::npos?

For some reason I was under the impression that the 1-arg form just
removed a single character, but I see that's incorrect.  Thanks.

Tom

[RFA 5/5] Use std::string rather than dyn-string

[Tom Tromey]

This patch changes some code in cli-cmds.c to use std::string rather
than dyn-string, removing some cleanups.  Since this was the last use
of dyn-string in gdb, this patch also removes
make_cleanup_dyn_string_delete.

2016-09-22  Tom Tromey  <tom@tromey.com>

	* utils.h (make_cleanup_dyn_string_delete): Remove declaration.
	* utils.c: Don't include dyn-string.h.
	(do_dyn_string_delete, make_cleanup_dyn_string_delete): Remove.
	* cli/cli-cmds.c: Include <string>.  Don't include dyn-string.h.
	(argv_to_string): Rename.  Change return type to std::string.
	(alias_command): Use std::string.
---
 gdb/ChangeLog      |  9 +++++++++
 gdb/cli/cli-cmds.c | 41 ++++++++++++++++++-----------------------
 gdb/utils.c        | 13 -------------
 gdb/utils.h        |  3 ---
 4 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 92e79c3..037eb02 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2016-09-22  Tom Tromey  <tom@tromey.com>
 
+	* utils.h (make_cleanup_dyn_string_delete): Remove declaration.
+	* utils.c: Don't include dyn-string.h.
+	(do_dyn_string_delete, make_cleanup_dyn_string_delete): Remove.
+	* cli/cli-cmds.c: Include <string>.  Don't include dyn-string.h.
+	(argv_to_string): Rename.  Change return type to std::string.
+	(alias_command): Use std::string.
+
+2016-09-22  Tom Tromey  <tom@tromey.com>
+
 	* objfiles.c: Include <vector>.
 	(objfile_relocate): Use std::vector.
 
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 384a336..8c3bdb2 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -19,7 +19,6 @@
 
 #include "defs.h"
 #include "arch-utils.h"
-#include "dyn-string.h"
 #include "readline/readline.h"
 #include "readline/tilde.h"
 #include "completer.h"
@@ -57,6 +56,7 @@
 
 #include <fcntl.h>
 #include <algorithm>
+#include <string>
 
 /* Prototypes for local command functions */
 
@@ -1382,11 +1382,11 @@ apropos_command (char *searchstr, int from_tty)
    This does not take care of quoting elements in case they contain spaces
    on purpose.  */
 
-static dyn_string_t
-argv_to_dyn_string (char **argv, int n)
+static std::string
+argv_to_string (char **argv, int n)
 {
   int i;
-  dyn_string_t result = dyn_string_new (10);
+  std::string result;
 
   gdb_assert (argv != NULL);
   gdb_assert (n >= 0 && n <= countargv (argv));
@@ -1394,8 +1394,8 @@ argv_to_dyn_string (char **argv, int n)
   for (i = 0; i < n; ++i)
     {
       if (i > 0)
-	dyn_string_append_char (result, ' ');
-      dyn_string_append_cstr (result, argv[i]);
+	result += " ";
+      result += argv[i];
     }
 
   return result;
@@ -1437,9 +1437,10 @@ alias_command (char *args, int from_tty)
 {
   int i, alias_argc, command_argc;
   int abbrev_flag = 0;
-  char *args2, *equals, *alias, *command;
+  char *args2, *equals;
+  const char *alias, *command;
   char **alias_argv, **command_argv;
-  dyn_string_t alias_dyn_string, command_dyn_string;
+  std::string alias_string, command_string;
   struct cleanup *cleanup;
 
   if (args == NULL || strchr (args, '=') == NULL)
@@ -1491,16 +1492,14 @@ alias_command (char *args, int from_tty)
   /* COMMAND must exist.
      Reconstruct the command to remove any extraneous spaces,
      for better error messages.  */
-  command_dyn_string = argv_to_dyn_string (command_argv, command_argc);
-  make_cleanup_dyn_string_delete (command_dyn_string);
-  command = dyn_string_buf (command_dyn_string);
+  command_string = argv_to_string (command_argv, command_argc);
+  command = command_string.c_str ();
   if (! valid_command_p (command))
     error (_("Invalid command to alias to: %s"), command);
 
   /* ALIAS must not exist.  */
-  alias_dyn_string = argv_to_dyn_string (alias_argv, alias_argc);
-  make_cleanup_dyn_string_delete (alias_dyn_string);
-  alias = dyn_string_buf (alias_dyn_string);
+  alias_string = argv_to_string (alias_argv, alias_argc);
+  alias = alias_string.c_str ();
   if (valid_command_p (alias))
     error (_("Alias already exists: %s"), alias);
 
@@ -1521,7 +1520,7 @@ alias_command (char *args, int from_tty)
     }
   else
     {
-      dyn_string_t alias_prefix_dyn_string, command_prefix_dyn_string;
+      std::string alias_prefix_string, command_prefix_string;
       const char *alias_prefix, *command_prefix;
       struct cmd_list_element *c_alias, *c_command;
 
@@ -1530,14 +1529,10 @@ alias_command (char *args, int from_tty)
 
       /* Create copies of ALIAS and COMMAND without the last word,
 	 and use that to verify the leading elements match.  */
-      alias_prefix_dyn_string =
-	argv_to_dyn_string (alias_argv, alias_argc - 1);
-      make_cleanup_dyn_string_delete (alias_prefix_dyn_string);
-      command_prefix_dyn_string =
-	argv_to_dyn_string (alias_argv, command_argc - 1);
-      make_cleanup_dyn_string_delete (command_prefix_dyn_string);
-      alias_prefix = dyn_string_buf (alias_prefix_dyn_string);
-      command_prefix = dyn_string_buf (command_prefix_dyn_string);
+      alias_prefix_string = argv_to_string (alias_argv, alias_argc - 1);
+      command_prefix_string = argv_to_string (alias_argv, command_argc - 1);
+      alias_prefix = alias_prefix_string.c_str ();
+      command_prefix = command_prefix_string.c_str ();
 
       c_command = lookup_cmd_1 (& command_prefix, cmdlist, NULL, 1);
       /* We've already tried to look up COMMAND.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index 2afff80..af2c79b 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -18,7 +18,6 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
-#include "dyn-string.h"
 #include <ctype.h>
 #include "gdb_wait.h"
 #include "event-top.h"
@@ -155,18 +154,6 @@ make_cleanup_freeargv (char **arg)
 }
 
 static void
-do_dyn_string_delete (void *arg)
-{
-  dyn_string_delete ((dyn_string_t) arg);
-}
-
-struct cleanup *
-make_cleanup_dyn_string_delete (dyn_string_t arg)
-{
-  return make_cleanup (do_dyn_string_delete, arg);
-}
-
-static void
 do_bfd_close_cleanup (void *arg)
 {
   gdb_bfd_unref ((bfd *) arg);
diff --git a/gdb/utils.h b/gdb/utils.h
index bf77d7d..8635075 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -64,9 +64,6 @@ char **gdb_buildargv (const char *);
 
 extern struct cleanup *make_cleanup_freeargv (char **);
 
-struct dyn_string;
-extern struct cleanup *make_cleanup_dyn_string_delete (struct dyn_string *);
-
 struct ui_file;
 extern struct cleanup *make_cleanup_ui_file_delete (struct ui_file *);
 
-- 
2.7.4

Re: [RFA 0/5] Some random C++-ification

[Pedro Alves]

On 09/22/2016 06:50 PM, Tom Tromey wrote:
> I was feeling inspired by Pedro's Cauldron slides, so I found a few

Awesome!  :-)

> random spots that could be converted from cleanups to self-managing
> data structures from libstdc++ -- in these cases, std::string and
> std::vector.
> 
> I saw a note in one of the C++ conversion documents about perhaps not
> using std::vector, since GCC did not.  However, I think often GCC's
> uses are unusual, and I don't think there is any reason to avoid
> std::vector in (most of) gdb.

Agreed.

> 
> This should probably not go in until after Pedro's "new" patch.

Does that patch look OK to you?

> 
> Built and regtested on x86-64 Fedora 24.
> 
> A few reflections on these changes:
> 
> * First, I think it's a nice improvement.  The examples here aren't so
>   dramatic, but if you dig a bit it's easy to find cases where the
>   cleanup logic is complicated; and this approach eventually lets one
>   delegate all that work to the compiler.

Agreed.

> 
> * Speaking of, I have a patch to convert uses of the ensure_python_env
>   to use RAII.  However, I don't think this is ready to go in --
>   because the cleanups installed by this are order-sensitive with
>   respect to other cleanups that might be created in the various
>   Python-calling functions.  My belief is that cleanups have to all be
>   run before any destructors, so any ordering issues are a subtlety
>   that, in the short term, will have to be accounted for in code
>   review.
> 
> * It wasn't actually clear to me that this kind of change is
>   desirable.

I think it is.  We need to eliminate _all_ cleanups in order to
get rid of the TRY/CATCH macros...  I think there's a lot of
value in getting the codebase rid of the C -> C++ partial
transition and inviting to work on.

> 
> * I was unclear on the coding style to use so I just used the gdb C
>   style.  Maybe there are some spaces that shouldn't be there now.

Looked fine to me.

Thanks,
Pedro Alves

Re: [RFA 0/5] Some random C++-ification

[Pedro Alves]

For to say that apart from the small comments I sent to
patches #2 and #3, it all looked good to me.

Thanks,
Pedro Alves

Re: [RFA 0/5] Some random C++-ification

[Trevor Saunders]

On Thu, Sep 22, 2016 at 08:08:20PM +0100, Pedro Alves wrote:
> On 09/22/2016 06:50 PM, Tom Tromey wrote:
> > I was feeling inspired by Pedro's Cauldron slides, so I found a few
> 
> Awesome!  :-)
> 
> > random spots that could be converted from cleanups to self-managing
> > data structures from libstdc++ -- in these cases, std::string and
> > std::vector.
> > 
> > I saw a note in one of the C++ conversion documents about perhaps not
> > using std::vector, since GCC did not.  However, I think often GCC's
> > uses are unusual, and I don't think there is any reason to avoid
> > std::vector in (most of) gdb.
> 
> Agreed.

yeah, gcc has the "reason" of needing vec to work with gc.  gdb might
want the perf advantage in auto_vec of using stack storage for short
arrays, but that's the only reason I can see to not use std::vector
outside of heap data structures.  In the heap I think  the auto_vec
layout is better than std::vector, but again just a perf question.

Trev

Re: [RFA 0/5] Some random C++-ification

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> This should probably not go in until after Pedro's "new" patch.

Pedro> Does that patch look OK to you?

I read it just now and I think it looks fine.

Tom

Re: [RFA 0/5] Some random C++-ification

[Pedro Alves]

On 09/23/2016 04:43 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Tom> This should probably not go in until after Pedro's "new" patch.
> 
> Pedro> Does that patch look OK to you?
> 
> I read it just now and I think it looks fine.

Great, thanks!

I've pushed it in now (with a couple typos fixed in the commit log).

Thanks,
Pedro Alves