[RFA 0/3] some C++-ification in breakpoint.c

[RFA 0/3] some C++-ification in breakpoint.c

[Tom Tromey]

This series adds a bit more C++-ification to breakpoint.c.

In particular it changes bpstats to be allocated with new, and changes
them to have constructors and destructors; changes some code in
breakpoint.c to use function_view; and then finally removes
counted_command_line in favor of std::shared_ptr.

Regression tested on the buildbot.

Tom

[RFA 3/3] Change counted_command_line to a shared_ptr

[Tom Tromey]

This changes counted_command_line to be a typedef for std::shared_ptr
and removes the associated cleanups.  In the long run I believe that
cmd_list_element should also be changed to use a shared_ptr.

ChangeLog
2017-08-30  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (struct counted_command_line): Remove.
	(breakpoint_commands): Update.
	(alloc_counted_command_line, incref_counted_command_line)
	(decref_counted_command_line, do_cleanup_counted_command_line)
	(make_cleanup_decref_counted_command_line): Remove.
	(breakpoint_set_commands, commands_command_1, ~bpstats, bpstats)
	(bpstat_clear_actions, bpstat_do_actions_1, watchpoint_check)
	(bpstat_stop_status, print_one_breakpoint_location, ~breakpoint)
	(save_breakpoints): Update.
	* breakpoint.h (counted_command_line): Now a typedef to
	shared_ptr.
	(struct breakpoint) <commands>: Now a counted_command_line.
	(struct bpstats) <command>: Likewise.
---
 gdb/ChangeLog    |  16 +++++++
 gdb/breakpoint.c | 130 ++++++++-----------------------------------------------
 gdb/breakpoint.h |   9 ++--
 3 files changed, 39 insertions(+), 116 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 28de155..0816cdd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,21 @@
 2017-08-30  Tom Tromey  <tom@tromey.com>
 
+	* breakpoint.c (struct counted_command_line): Remove.
+	(breakpoint_commands): Update.
+	(alloc_counted_command_line, incref_counted_command_line)
+	(decref_counted_command_line, do_cleanup_counted_command_line)
+	(make_cleanup_decref_counted_command_line): Remove.
+	(breakpoint_set_commands, commands_command_1, ~bpstats, bpstats)
+	(bpstat_clear_actions, bpstat_do_actions_1, watchpoint_check)
+	(bpstat_stop_status, print_one_breakpoint_location, ~breakpoint)
+	(save_breakpoints): Update.
+	* breakpoint.h (counted_command_line): Now a typedef to
+	shared_ptr.
+	(struct breakpoint) <commands>: Now a counted_command_line.
+	(struct bpstats) <command>: Likewise.
+
+2017-08-30  Tom Tromey  <tom@tromey.com>
+
 	* breakpoint.c (struct commands_info, do_map_commands_command):
 	Remove.
 	(commands_command_1): Update.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fd28f7c..72de69a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -358,21 +358,10 @@ static char *dprintf_channel;
    has disconnected.  */
 static int disconnected_dprintf = 1;
 
-/* A reference-counted struct command_line.  This lets multiple
-   breakpoints share a single command list.  */
-struct counted_command_line
-{
-  /* The reference count.  */
-  int refc;
-
-  /* The command list.  */
-  struct command_line *commands;
-};
-
 struct command_line *
 breakpoint_commands (struct breakpoint *b)
 {
-  return b->commands ? b->commands->commands : NULL;
+  return b->commands ? b->commands.get () : NULL;
 }
 
 /* Flag indicating that a command has proceeded the inferior past the
@@ -718,64 +707,6 @@ clear_breakpoint_hit_counts (void)
     b->hit_count = 0;
 }
 
-/* Allocate a new counted_command_line with reference count of 1.
-   The new structure owns COMMANDS.  */
-
-static struct counted_command_line *
-alloc_counted_command_line (struct command_line *commands)
-{
-  struct counted_command_line *result = XNEW (struct counted_command_line);
-
-  result->refc = 1;
-  result->commands = commands;
-
-  return result;
-}
-
-/* Increment reference count.  This does nothing if CMD is NULL.  */
-
-static void
-incref_counted_command_line (struct counted_command_line *cmd)
-{
-  if (cmd)
-    ++cmd->refc;
-}
-
-/* Decrement reference count.  If the reference count reaches 0,
-   destroy the counted_command_line.  Sets *CMDP to NULL.  This does
-   nothing if *CMDP is NULL.  */
-
-static void
-decref_counted_command_line (struct counted_command_line **cmdp)
-{
-  if (*cmdp)
-    {
-      if (--(*cmdp)->refc == 0)
-	{
-	  free_command_lines (&(*cmdp)->commands);
-	  xfree (*cmdp);
-	}
-      *cmdp = NULL;
-    }
-}
-
-/* A cleanup function that calls decref_counted_command_line.  */
-
-static void
-do_cleanup_counted_command_line (void *arg)
-{
-  decref_counted_command_line ((struct counted_command_line **) arg);
-}
-
-/* Create a cleanup that calls decref_counted_command_line on the
-   argument.  */
-
-static struct cleanup *
-make_cleanup_decref_counted_command_line (struct counted_command_line **cmdp)
-{
-  return make_cleanup (do_cleanup_counted_command_line, cmdp);
-}
-
 \f
 /* Return the breakpoint with the specified number, or NULL
    if the number does not refer to an existing breakpoint.  */
@@ -1304,8 +1235,7 @@ breakpoint_set_commands (struct breakpoint *b,
 {
   validate_commands_for_breakpoint (b, commands.get ());
 
-  decref_counted_command_line (&b->commands);
-  b->commands = alloc_counted_command_line (commands.release ());
+  b->commands = std::move (commands);
   observer_notify_breakpoint_modified (b);
 }
 
@@ -1361,12 +1291,7 @@ static void
 commands_command_1 (const char *arg, int from_tty,
 		    struct command_line *control)
 {
-  struct cleanup *cleanups;
-  struct counted_command_line *cmd = NULL;
-
-  /* If we read command lines from the user, then `info' will hold an
-     extra reference to the commands that we must clean up.  */
-  cleanups = make_cleanup_decref_counted_command_line (&cmd);
+  counted_command_line cmd;
 
   std::string new_arg;
 
@@ -1385,10 +1310,8 @@ commands_command_1 (const char *arg, int from_tty,
      {
        if (cmd == NULL)
 	 {
-	   command_line_up l;
-
 	   if (control != NULL)
-	     l = copy_command_lines (control->body_list[0]);
+	     cmd = copy_command_lines (control->body_list[0]);
 	   else
 	     {
 	       struct cleanup *old_chain;
@@ -1400,25 +1323,21 @@ commands_command_1 (const char *arg, int from_tty,
 
 	       old_chain = make_cleanup (xfree, str);
 
-	       l = read_command_lines (str,
-				       from_tty, 1,
-				       (is_tracepoint (b)
-					? check_tracepoint_command : 0),
-				       b);
+	       cmd = read_command_lines (str,
+					 from_tty, 1,
+					 (is_tracepoint (b)
+					  ? check_tracepoint_command : 0),
+					 b);
 
 	       do_cleanups (old_chain);
 	     }
-
-	   cmd = alloc_counted_command_line (l.release ());
 	 }
 
        /* If a breakpoint was on the list more than once, we don't need to
 	  do anything.  */
        if (b->commands != cmd)
 	 {
-	   validate_commands_for_breakpoint (b, cmd->commands);
-	   incref_counted_command_line (cmd);
-	   decref_counted_command_line (&b->commands);
+	   validate_commands_for_breakpoint (b, cmd.get ());
 	   b->commands = cmd;
 	   observer_notify_breakpoint_modified (b);
 	 }
@@ -1426,8 +1345,6 @@ commands_command_1 (const char *arg, int from_tty,
 
   if (cmd == NULL)
     error (_("No breakpoints specified."));
-
-  do_cleanups (cleanups);
 }
 
 static void
@@ -4428,7 +4345,6 @@ bpstats::~bpstats ()
 {
   if (old_val != NULL)
     value_free (old_val);
-  decref_counted_command_line (&commands);
   if (bp_location_at != NULL)
     decref_bp_location (&bp_location_at);
 }
@@ -4470,7 +4386,6 @@ bpstats::bpstats (const bpstats &other)
       release_value (old_val);
     }
   incref_bp_location (bp_location_at);
-  incref_counted_command_line (commands);
 }
 
 /* Return a copy of a bpstat.  Like "bs1 = bs2" but all storage that
@@ -4589,7 +4504,7 @@ bpstat_clear_actions (void)
 
   for (bs = tp->control.stop_bpstat; bs != NULL; bs = bs->next)
     {
-      decref_counted_command_line (&bs->commands);
+      bs->commands = NULL;
 
       if (bs->old_val != NULL)
 	{
@@ -4668,9 +4583,7 @@ bpstat_do_actions_1 (bpstat *bsp)
   breakpoint_proceeded = 0;
   for (; bs != NULL; bs = bs->next)
     {
-      struct counted_command_line *ccmd;
-      struct command_line *cmd;
-      struct cleanup *this_cmd_tree_chain;
+      struct command_line *cmd = NULL;
 
       /* Take ownership of the BSP's command tree, if it has one.
 
@@ -4682,10 +4595,10 @@ bpstat_do_actions_1 (bpstat *bsp)
          commands are only executed once, we don't need to copy it; we
          can clear the pointer in the bpstat, and make sure we free
          the tree when we're done.  */
-      ccmd = bs->commands;
+      counted_command_line ccmd = bs->commands;
       bs->commands = NULL;
-      this_cmd_tree_chain = make_cleanup_decref_counted_command_line (&ccmd);
-      cmd = ccmd ? ccmd->commands : NULL;
+      if (ccmd != NULL)
+	cmd = ccmd.get ();
       if (command_line_is_silent (cmd))
 	{
 	  /* The action has been already done by bpstat_stop_status.  */
@@ -4702,9 +4615,6 @@ bpstat_do_actions_1 (bpstat *bsp)
 	    cmd = cmd->next;
 	}
 
-      /* We can free this command tree now.  */
-      do_cleanups (this_cmd_tree_chain);
-
       if (breakpoint_proceeded)
 	{
 	  if (current_ui->async)
@@ -5240,7 +5150,7 @@ watchpoint_check (void *p)
 	}
 
       /* Make sure the watchpoint's commands aren't executed.  */
-      decref_counted_command_line (&b->commands);
+      b->commands = NULL;
       watchpoint_del_at_next_stop (b);
 
       return WP_DELETED;
@@ -5730,9 +5640,8 @@ bpstat_stop_status (struct address_space *aspace,
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
 	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
+					  ? bs->commands.get () : NULL))
 		bs->print = 0;
 
 	      b->ops->after_condition_true (bs);
@@ -6595,7 +6504,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 	}
     }
 
-  l = b->commands ? b->commands->commands : NULL;
+  l = b->commands ? b->commands.get () : NULL;
   if (!part_of_multiple && l)
     {
       annotate_field (9);
@@ -12674,7 +12583,6 @@ static const struct bp_location_ops bp_location_ops =
 
 breakpoint::~breakpoint ()
 {
-  decref_counted_command_line (&this->commands);
   xfree (this->cond_string);
   xfree (this->extra_string);
   xfree (this->filter);
@@ -15454,7 +15362,7 @@ save_breakpoints (char *filename, int from_tty,
 	current_uiout->redirect (&fp);
 	TRY
 	  {
-	    print_command_lines (current_uiout, tp->commands->commands, 2);
+	    print_command_lines (current_uiout, tp->commands.get (), 2);
 	  }
 	CATCH (ex, RETURN_MASK_ALL)
 	  {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f3c0961..ae7547e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -655,10 +655,9 @@ enum watchpoint_triggered
 typedef struct bp_location *bp_location_p;
 DEF_VEC_P(bp_location_p);
 
-/* A reference-counted struct command_line.  This lets multiple
-   breakpoints share a single command list.  This is an implementation
+/* A reference-counted struct command_line. This is an implementation
    detail to the breakpoints module.  */
-struct counted_command_line;
+typedef std::shared_ptr<command_line> counted_command_line;
 
 /* Some targets (e.g., embedded PowerPC) need two debug registers to set
    a watchpoint over a memory region.  If this flag is true, GDB will use
@@ -710,7 +709,7 @@ struct breakpoint
 
   /* Chain of command lines to execute when this breakpoint is
      hit.  */
-  counted_command_line *commands = NULL;
+  counted_command_line commands;
   /* Stack depth (address of frame).  If nonzero, break only if fp
      equals this.  */
   struct frame_id frame_id = null_frame_id;
@@ -1116,7 +1115,7 @@ struct bpstats
     struct breakpoint *breakpoint_at;
 
     /* The associated command list.  */
-    struct counted_command_line *commands;
+    counted_command_line commands;
 
     /* Old value associated with a watchpoint.  */
     struct value *old_val;
-- 
2.9.4

[RFA 1/3] Allocate bpstats with new

[Tom Tromey]

This changes struct bpstats to be allocated with new and freed with
delete, adding constructors and a destructor in the process.  This
allows the removal of one cleanup and clears the way for more to
follow.

ChangeLog
2017-08-30  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (~bpstats): Rename from bpstat_free.  Update.
	(bpstat_clear): Use delete.
	(bpstats): New constructors.
	(bpstat_copy, bpstat_stop_status): Use new.
	(dprintf_after_condition_true): Update.
	* breakpoint.h (bpstats::bpstats): Add constructors.
	(bpstats::~bpstats): Add destructor.
---
 gdb/ChangeLog    | 10 +++++++
 gdb/breakpoint.c | 91 +++++++++++++++++++++++++++++++++-----------------------
 gdb/breakpoint.h |  7 +++++
 3 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5559bc2..5baad20 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2017-08-30  Tom Tromey  <tom@tromey.com>
+
+	* breakpoint.c (~bpstats): Rename from bpstat_free.  Update.
+	(bpstat_clear): Use delete.
+	(bpstats): New constructors.
+	(bpstat_copy, bpstat_stop_status): Use new.
+	(dprintf_after_condition_true): Update.
+	* breakpoint.h (bpstats::bpstats): Add constructors.
+	(bpstats::~bpstats): Add destructor.
+
 2017-08-29  John Baldwin  <jhb@FreeBSD.org>
 
 	* mips-fbsd-nat.c (getfpregs_supplies): Return true for FIR.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ae09da4..57e8479 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4459,14 +4459,13 @@ is_catchpoint (struct breakpoint *ep)
 /* Frees any storage that is part of a bpstat.  Does not walk the
    'next' chain.  */
 
-static void
-bpstat_free (bpstat bs)
+bpstats::~bpstats ()
 {
-  if (bs->old_val != NULL)
-    value_free (bs->old_val);
-  decref_counted_command_line (&bs->commands);
-  decref_bp_location (&bs->bp_location_at);
-  xfree (bs);
+  if (old_val != NULL)
+    value_free (old_val);
+  decref_counted_command_line (&commands);
+  if (bp_location_at != NULL)
+    decref_bp_location (&bp_location_at);
 }
 
 /* Clear a bpstat so that it says we are not at any breakpoint.
@@ -4484,12 +4483,31 @@ bpstat_clear (bpstat *bsp)
   while (p != NULL)
     {
       q = p->next;
-      bpstat_free (p);
+      delete p;
       p = q;
     }
   *bsp = NULL;
 }
 
+bpstats::bpstats (const bpstats &other)
+  : next (NULL),
+    bp_location_at (other.bp_location_at),
+    breakpoint_at (other.breakpoint_at),
+    commands (other.commands),
+    old_val (other.old_val),
+    print (other.print),
+    stop (other.stop),
+    print_it (other.print_it)
+{
+  if (old_val != NULL)
+    {
+      old_val = value_copy (old_val);
+      release_value (old_val);
+    }
+  incref_bp_location (bp_location_at);
+  incref_counted_command_line (commands);
+}
+
 /* Return a copy of a bpstat.  Like "bs1 = bs2" but all storage that
    is part of the bpstat is copied as well.  */
 
@@ -4505,15 +4523,7 @@ bpstat_copy (bpstat bs)
 
   for (; bs != NULL; bs = bs->next)
     {
-      tmp = (bpstat) xmalloc (sizeof (*tmp));
-      memcpy (tmp, bs, sizeof (*tmp));
-      incref_counted_command_line (tmp->commands);
-      incref_bp_location (tmp->bp_location_at);
-      if (bs->old_val != NULL)
-	{
-	  tmp->old_val = value_copy (bs->old_val);
-	  release_value (tmp->old_val);
-	}
+      tmp = new bpstats (*bs);
 
       if (p == NULL)
 	/* This is the first thing in the chain.  */
@@ -5002,23 +5012,31 @@ breakpoint_cond_eval (void *exp)
 
 /* Allocate a new bpstat.  Link it to the FIFO list by BS_LINK_POINTER.  */
 
-static bpstat
-bpstat_alloc (struct bp_location *bl, bpstat **bs_link_pointer)
+bpstats::bpstats (struct bp_location *bl, bpstat **bs_link_pointer)
+  : next (NULL),
+    bp_location_at (bl),
+    breakpoint_at (bl->owner),
+    commands (NULL),
+    old_val (NULL),
+    print (0),
+    stop (0),
+    print_it (print_it_normal)
 {
-  bpstat bs;
-
-  bs = (bpstat) xmalloc (sizeof (*bs));
-  bs->next = NULL;
-  **bs_link_pointer = bs;
-  *bs_link_pointer = &bs->next;
-  bs->breakpoint_at = bl->owner;
-  bs->bp_location_at = bl;
   incref_bp_location (bl);
-  /* If the condition is false, etc., don't do the commands.  */
-  bs->commands = NULL;
-  bs->old_val = NULL;
-  bs->print_it = print_it_normal;
-  return bs;
+  **bs_link_pointer = this;
+  *bs_link_pointer = &next;
+}
+
+bpstats::bpstats ()
+  : next (NULL),
+    bp_location_at (NULL),
+    breakpoint_at (NULL),
+    commands (NULL),
+    old_val (NULL),
+    print (0),
+    stop (0),
+    print_it (print_it_normal)
+{
 }
 \f
 /* The target has stopped with waitstatus WS.  Check if any hardware
@@ -5661,7 +5679,7 @@ bpstat_stop_status (struct address_space *aspace,
 	  /* Come here if it's a watchpoint, or if the break address
 	     matches.  */
 
-	  bs = bpstat_alloc (bl, &bs_link);	/* Alloc a bpstat to
+	  bs = new bpstats (bl, &bs_link);	/* Alloc a bpstat to
 						   explain stop.  */
 
 	  /* Assume we stop.  Should we find a watchpoint that is not
@@ -5692,7 +5710,7 @@ bpstat_stop_status (struct address_space *aspace,
 	  if (breakpoint_location_address_match (loc, aspace, bp_addr)
 	      && need_moribund_for_location_type (loc))
 	    {
-	      bs = bpstat_alloc (loc, &bs_link);
+	      bs = new bpstats (loc, &bs_link);
 	      /* For hits of moribund locations, we should just proceed.  */
 	      bs->stop = 0;
 	      bs->print = 0;
@@ -13466,8 +13484,7 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
 static void
 dprintf_after_condition_true (struct bpstats *bs)
 {
-  struct cleanup *old_chain;
-  struct bpstats tmp_bs = { NULL };
+  struct bpstats tmp_bs;
   struct bpstats *tmp_bs_p = &tmp_bs;
 
   /* dprintf's never cause a stop.  This wasn't set in the
@@ -13482,14 +13499,12 @@ dprintf_after_condition_true (struct bpstats *bs)
      commands here throws.  */
   tmp_bs.commands = bs->commands;
   bs->commands = NULL;
-  old_chain = make_cleanup_decref_counted_command_line (&tmp_bs.commands);
 
   bpstat_do_actions_1 (&tmp_bs_p);
 
   /* 'tmp_bs.commands' will usually be NULL by now, but
      bpstat_do_actions_1 may return early without processing the whole
      list.  */
-  do_cleanups (old_chain);
 }
 
 /* The breakpoint_ops structure to be used on static tracepoints with
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index dc2b098..f3c0961 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1080,6 +1080,13 @@ enum bp_print_how
 
 struct bpstats
   {
+    bpstats ();
+    bpstats (struct bp_location *bl, bpstat **bs_link_pointer);
+    ~bpstats ();
+
+    bpstats (const bpstats &);
+    bpstats &operator= (const bpstats &) = delete;
+
     /* Linked list because there can be more than one breakpoint at
        the same place, and a bpstat reflects the fact that all have
        been hit.  */
-- 
2.9.4

[RFA 2/3] Use function_view in a couple of places in breakpoint.c

[Tom Tromey]

This changes iterate_over_related_breakpoints and
map_breakpoint_numbers to take a function_view.  Then, it simplifies
the callers by using lambdas.  This then allows the removal of some
bookkeeping types.

ChangeLog
2017-08-30  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (struct commands_info, do_map_commands_command):
	Remove.
	(commands_command_1): Update.
	(iterate_over_related_breakpoints): Take a function_view.
	(do_delete_breakpoint, do_map_delete_breakpoint): Remove.
	(delete_command): Update.
	(map_breakpoint_numbers): Take a function_view.
	(do_disable_breakpoint, do_map_delete_breakpoint): Remove.
	(disable_command): Update.
	(do_enable_breakpoint, do_map_enable_breakpoint): Remove.
	(enable_command): Update.
	(struct disp_data, do_enable_breakpoint_disp)
	(do_map_enable_once_breakpoint, do_map_enable_count_breakpoint)
	(do_map_enable_delete_breakpoint): Remove.
	(enable_once_command, enable_count_command, enable_delete_command)
	(delete_trace_variable_command): Update.
---
 gdb/ChangeLog    |  19 ++++
 gdb/breakpoint.c | 291 +++++++++++++++++++------------------------------------
 2 files changed, 121 insertions(+), 189 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5baad20..28de155 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,24 @@
 2017-08-30  Tom Tromey  <tom@tromey.com>
 
+	* breakpoint.c (struct commands_info, do_map_commands_command):
+	Remove.
+	(commands_command_1): Update.
+	(iterate_over_related_breakpoints): Take a function_view.
+	(do_delete_breakpoint, do_map_delete_breakpoint): Remove.
+	(delete_command): Update.
+	(map_breakpoint_numbers): Take a function_view.
+	(do_disable_breakpoint, do_map_delete_breakpoint): Remove.
+	(disable_command): Update.
+	(do_enable_breakpoint, do_map_enable_breakpoint): Remove.
+	(enable_command): Update.
+	(struct disp_data, do_enable_breakpoint_disp)
+	(do_map_enable_once_breakpoint, do_map_enable_count_breakpoint)
+	(do_map_enable_delete_breakpoint): Remove.
+	(enable_once_command, enable_count_command, enable_delete_command)
+	(delete_trace_variable_command): Update.
+
+2017-08-30  Tom Tromey  <tom@tromey.com>
+
 	* breakpoint.c (~bpstats): Rename from bpstat_free.  Update.
 	(bpstat_clear): Use delete.
 	(bpstats): New constructors.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 57e8479..fd28f7c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -103,9 +103,7 @@ static void disable_command (char *, int);
 static void enable_command (char *, int);
 
 static void map_breakpoint_numbers (const char *,
-				    void (*) (struct breakpoint *,
-					      void *),
-				    void *);
+				    gdb::function_view<void (breakpoint *)>);
 
 static void ignore_command (char *, int);
 
@@ -1359,88 +1357,16 @@ check_tracepoint_command (char *line, void *closure)
   validate_actionline (line, b);
 }
 
-/* A structure used to pass information through
-   map_breakpoint_numbers.  */
-
-struct commands_info
-{
-  /* True if the command was typed at a tty.  */
-  int from_tty;
-
-  /* The breakpoint range spec.  */
-  const char *arg;
-
-  /* Non-NULL if the body of the commands are being read from this
-     already-parsed command.  */
-  struct command_line *control;
-
-  /* The command lines read from the user, or NULL if they have not
-     yet been read.  */
-  struct counted_command_line *cmd;
-};
-
-/* A callback for map_breakpoint_numbers that sets the commands for
-   commands_command.  */
-
-static void
-do_map_commands_command (struct breakpoint *b, void *data)
-{
-  struct commands_info *info = (struct commands_info *) data;
-
-  if (info->cmd == NULL)
-    {
-      command_line_up l;
-
-      if (info->control != NULL)
-	l = copy_command_lines (info->control->body_list[0]);
-      else
-	{
-	  struct cleanup *old_chain;
-	  char *str;
-
-	  str = xstrprintf (_("Type commands for breakpoint(s) "
-			      "%s, one per line."),
-			    info->arg);
-
-	  old_chain = make_cleanup (xfree, str);
-
-	  l = read_command_lines (str,
-				  info->from_tty, 1,
-				  (is_tracepoint (b)
-				   ? check_tracepoint_command : 0),
-				  b);
-
-	  do_cleanups (old_chain);
-	}
-
-      info->cmd = alloc_counted_command_line (l.release ());
-    }
-
-  /* If a breakpoint was on the list more than once, we don't need to
-     do anything.  */
-  if (b->commands != info->cmd)
-    {
-      validate_commands_for_breakpoint (b, info->cmd->commands);
-      incref_counted_command_line (info->cmd);
-      decref_counted_command_line (&b->commands);
-      b->commands = info->cmd;
-      observer_notify_breakpoint_modified (b);
-    }
-}
-
 static void
 commands_command_1 (const char *arg, int from_tty,
 		    struct command_line *control)
 {
   struct cleanup *cleanups;
-  struct commands_info info;
+  struct counted_command_line *cmd = NULL;
 
-  info.from_tty = from_tty;
-  info.control = control;
-  info.cmd = NULL;
   /* If we read command lines from the user, then `info' will hold an
      extra reference to the commands that we must clean up.  */
-  cleanups = make_cleanup_decref_counted_command_line (&info.cmd);
+  cleanups = make_cleanup_decref_counted_command_line (&cmd);
 
   std::string new_arg;
 
@@ -1451,15 +1377,54 @@ commands_command_1 (const char *arg, int from_tty,
 				 breakpoint_count);
       else if (breakpoint_count > 0)
 	new_arg = string_printf ("%d", breakpoint_count);
-    }
-  else
-    new_arg = arg;
-
-  info.arg = new_arg.c_str ();
-
-  map_breakpoint_numbers (info.arg, do_map_commands_command, &info);
+      arg = new_arg.c_str ();
+    }
+
+  map_breakpoint_numbers
+    (arg, [&] (breakpoint *b)
+     {
+       if (cmd == NULL)
+	 {
+	   command_line_up l;
+
+	   if (control != NULL)
+	     l = copy_command_lines (control->body_list[0]);
+	   else
+	     {
+	       struct cleanup *old_chain;
+	       char *str;
+
+	       str = xstrprintf (_("Type commands for breakpoint(s) "
+				   "%s, one per line."),
+				 arg);
+
+	       old_chain = make_cleanup (xfree, str);
+
+	       l = read_command_lines (str,
+				       from_tty, 1,
+				       (is_tracepoint (b)
+					? check_tracepoint_command : 0),
+				       b);
+
+	       do_cleanups (old_chain);
+	     }
+
+	   cmd = alloc_counted_command_line (l.release ());
+	 }
+
+       /* If a breakpoint was on the list more than once, we don't need to
+	  do anything.  */
+       if (b->commands != cmd)
+	 {
+	   validate_commands_for_breakpoint (b, cmd->commands);
+	   incref_counted_command_line (cmd);
+	   decref_counted_command_line (&b->commands);
+	   b->commands = cmd;
+	   observer_notify_breakpoint_modified (b);
+	 }
+     });
 
-  if (info.cmd == NULL)
+  if (cmd == NULL)
     error (_("No breakpoints specified."));
 
   do_cleanups (cleanups);
@@ -13727,9 +13692,7 @@ make_cleanup_delete_breakpoint (struct breakpoint *b)
 
 static void
 iterate_over_related_breakpoints (struct breakpoint *b,
-				  void (*function) (struct breakpoint *,
-						    void *),
-				  void *data)
+				  gdb::function_view<void (breakpoint *)> function)
 {
   struct breakpoint *related;
 
@@ -13744,7 +13707,7 @@ iterate_over_related_breakpoints (struct breakpoint *b,
       if (next == related)
 	{
 	  /* RELATED is the last ring entry.  */
-	  function (related, data);
+	  function (related);
 
 	  /* FUNCTION may have deleted it, so we'd never reach back to
 	     B.  There's nothing left to do anyway, so just break
@@ -13752,28 +13715,13 @@ iterate_over_related_breakpoints (struct breakpoint *b,
 	  break;
 	}
       else
-	function (related, data);
+	function (related);
 
       related = next;
     }
   while (related != b);
 }
 
-static void
-do_delete_breakpoint (struct breakpoint *b, void *ignore)
-{
-  delete_breakpoint (b);
-}
-
-/* A callback for map_breakpoint_numbers that calls
-   delete_breakpoint.  */
-
-static void
-do_map_delete_breakpoint (struct breakpoint *b, void *ignore)
-{
-  iterate_over_related_breakpoints (b, do_delete_breakpoint, NULL);
-}
-
 void
 delete_command (char *arg, int from_tty)
 {
@@ -13805,7 +13753,11 @@ delete_command (char *arg, int from_tty)
 	}
     }
   else
-    map_breakpoint_numbers (arg, do_map_delete_breakpoint, NULL);
+    map_breakpoint_numbers
+      (arg, [&] (breakpoint *b)
+       {
+	 iterate_over_related_breakpoints (b, delete_breakpoint);
+       });
 }
 
 /* Return true if all locations of B bound to PSPACE are pending.  If
@@ -14540,9 +14492,7 @@ ignore_command (char *args, int from_tty)
 
 static void
 map_breakpoint_numbers (const char *args,
-			void (*function) (struct breakpoint *,
-					  void *),
-			void *data)
+			gdb::function_view<void (breakpoint *)> function)
 {
   int num;
   struct breakpoint *b, *tmp;
@@ -14568,7 +14518,7 @@ map_breakpoint_numbers (const char *args,
 	    if (b->number == num)
 	      {
 		match = true;
-		function (b, data);
+		function (b);
 		break;
 	      }
 	  if (!match)
@@ -14651,23 +14601,6 @@ disable_breakpoint (struct breakpoint *bpt)
   observer_notify_breakpoint_modified (bpt);
 }
 
-/* A callback for iterate_over_related_breakpoints.  */
-
-static void
-do_disable_breakpoint (struct breakpoint *b, void *ignore)
-{
-  disable_breakpoint (b);
-}
-
-/* A callback for map_breakpoint_numbers that calls
-   disable_breakpoint.  */
-
-static void
-do_map_disable_breakpoint (struct breakpoint *b, void *ignore)
-{
-  iterate_over_related_breakpoints (b, do_disable_breakpoint, NULL);
-}
-
 static void
 disable_command (char *args, int from_tty)
 {
@@ -14704,7 +14637,11 @@ disable_command (char *args, int from_tty)
 	      update_global_location_list (UGLL_DONT_INSERT);
 	    }
 	  else
-	    map_breakpoint_numbers (num, do_map_disable_breakpoint, NULL);
+	    map_breakpoint_numbers
+	      (num, [&] (breakpoint *b)
+	       {
+		 iterate_over_related_breakpoints (b, disable_breakpoint);
+	       });
 	  num = extract_arg (&args);
 	}
     }
@@ -14780,21 +14717,6 @@ enable_breakpoint (struct breakpoint *bpt)
   enable_breakpoint_disp (bpt, bpt->disposition, 0);
 }
 
-static void
-do_enable_breakpoint (struct breakpoint *bpt, void *arg)
-{
-  enable_breakpoint (bpt);
-}
-
-/* A callback for map_breakpoint_numbers that calls
-   enable_breakpoint.  */
-
-static void
-do_map_enable_breakpoint (struct breakpoint *b, void *ignore)
-{
-  iterate_over_related_breakpoints (b, do_enable_breakpoint, NULL);
-}
-
 /* The enable command enables the specified breakpoints (or all defined
    breakpoints) so they once again become (or continue to be) effective
    in stopping the inferior.  */
@@ -14835,49 +14757,28 @@ enable_command (char *args, int from_tty)
 	      update_global_location_list (UGLL_MAY_INSERT);
 	    }
 	  else
-	    map_breakpoint_numbers (num, do_map_enable_breakpoint, NULL);
+	    map_breakpoint_numbers
+	      (num, [&] (breakpoint *b)
+	       {
+		 iterate_over_related_breakpoints (b, enable_breakpoint);
+	       });
 	  num = extract_arg (&args);
 	}
     }
 }
 
-/* This struct packages up disposition data for application to multiple
-   breakpoints.  */
-
-struct disp_data
-{
-  enum bpdisp disp;
-  int count;
-};
-
-static void
-do_enable_breakpoint_disp (struct breakpoint *bpt, void *arg)
-{
-  struct disp_data disp_data = *(struct disp_data *) arg;
-
-  enable_breakpoint_disp (bpt, disp_data.disp, disp_data.count);
-}
-
-static void
-do_map_enable_once_breakpoint (struct breakpoint *bpt, void *ignore)
-{
-  struct disp_data disp = { disp_disable, 1 };
-
-  iterate_over_related_breakpoints (bpt, do_enable_breakpoint_disp, &disp);
-}
-
 static void
 enable_once_command (char *args, int from_tty)
 {
-  map_breakpoint_numbers (args, do_map_enable_once_breakpoint, NULL);
-}
-
-static void
-do_map_enable_count_breakpoint (struct breakpoint *bpt, void *countptr)
-{
-  struct disp_data disp = { disp_disable, *(int *) countptr };
-
-  iterate_over_related_breakpoints (bpt, do_enable_breakpoint_disp, &disp);
+  map_breakpoint_numbers
+    (args, [&] (breakpoint *b)
+     {
+       iterate_over_related_breakpoints
+	 (b, [&] (breakpoint *bpt)
+	  {
+	    enable_breakpoint_disp (bpt, disp_disable, 1);
+	  });
+     });
 }
 
 static void
@@ -14890,21 +14791,29 @@ enable_count_command (char *args, int from_tty)
 
   count = get_number (&args);
 
-  map_breakpoint_numbers (args, do_map_enable_count_breakpoint, &count);
-}
-
-static void
-do_map_enable_delete_breakpoint (struct breakpoint *bpt, void *ignore)
-{
-  struct disp_data disp = { disp_del, 1 };
-
-  iterate_over_related_breakpoints (bpt, do_enable_breakpoint_disp, &disp);
+  map_breakpoint_numbers
+    (args, [&] (breakpoint *b)
+     {
+       iterate_over_related_breakpoints
+	 (b, [&] (breakpoint *bpt)
+	  {
+	    enable_breakpoint_disp (bpt, disp_disable, count);
+	  });
+     });
 }
 
 static void
 enable_delete_command (char *args, int from_tty)
 {
-  map_breakpoint_numbers (args, do_map_enable_delete_breakpoint, NULL);
+  map_breakpoint_numbers
+    (args, [&] (breakpoint *b)
+     {
+       iterate_over_related_breakpoints
+	 (b, [&] (breakpoint *bpt)
+	  {
+	    enable_breakpoint_disp (bpt, disp_del, 1);
+	  });
+     });
 }
 \f
 static void
@@ -15304,7 +15213,11 @@ delete_trace_command (char *arg, int from_tty)
 	}
     }
   else
-    map_breakpoint_numbers (arg, do_map_delete_breakpoint, NULL);
+    map_breakpoint_numbers
+      (arg, [&] (breakpoint *b)
+       {
+	 iterate_over_related_breakpoints (b, delete_breakpoint);
+       });
 }
 
 /* Helper function for trace_pass_command.  */
-- 
2.9.4

Re: [RFA 3/3] Change counted_command_line to a shared_ptr

[Simon Marchi]

On 2017-08-31 00:06, Tom Tromey wrote:
> This changes counted_command_line to be a typedef for std::shared_ptr
> and removes the associated cleanups.  In the long run I believe that
> cmd_list_element should also be changed to use a shared_ptr.

The command_line object is currently freed using free_command_lines.  
The shared_ptr will "delete" it.  It seems to me that some C++ification 
of the command_line structure needs to happen for it to get de-allocated 
properly.

Simon

Re: [RFA 3/3] Change counted_command_line to a shared_ptr

[Simon Marchi]

On 2017-09-01 22:48, Simon Marchi wrote:
> On 2017-08-31 00:06, Tom Tromey wrote:
>> This changes counted_command_line to be a typedef for std::shared_ptr
>> and removes the associated cleanups.  In the long run I believe that
>> cmd_list_element should also be changed to use a shared_ptr.
> 
> The command_line object is currently freed using free_command_lines.
> The shared_ptr will "delete" it.  It seems to me that some
> C++ification of the command_line structure needs to happen for it to
> get de-allocated properly.

Hi Tom,

I looked at this more and found that free_command_lines was indeed being 
called when the shared_ptr free'd its pointer.  I finally understood 
that when assigning:

   a_shared_ptr = a_unique_ptr;

like commands_command_1 does, for example, the deleter is also 
transferred.

Simon

Re: [RFA 3/3] Change counted_command_line to a shared_ptr

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> I looked at this more and found that free_command_lines was indeed
Simon> being called when the shared_ptr free'd its pointer.  I finally
Simon> understood that when assigning:
Simon>   a_shared_ptr = a_unique_ptr;
Simon> like commands_command_1 does, for example, the deleter is also
Simon> transferred.

Yeah, I should probably have mentioned that.  I had to read about it
too, but then I think I had forgotten about it by the time I sent the
patch.  I don't really understand why unique_ptr and shared_ptr differ
in this way; if you know the rationale I'd be interested in learning it.

Tom

Re: [RFA 3/3] Change counted_command_line to a shared_ptr

[Simon Marchi]

On 2017-09-02 17:07, Tom Tromey wrote:
> Simon> I looked at this more and found that free_command_lines was 
> indeed
> Simon> being called when the shared_ptr free'd its pointer.  I finally
> Simon> understood that when assigning:
> Simon>   a_shared_ptr = a_unique_ptr;
> Simon> like commands_command_1 does, for example, the deleter is also
> Simon> transferred.
> 
> Yeah, I should probably have mentioned that.  I had to read about it
> too, but then I think I had forgotten about it by the time I sent the
> patch.  I don't really understand why unique_ptr and shared_ptr differ
> in this way; if you know the rationale I'd be interested in learning 
> it.

 From what I read, unique_ptr accepts its deleter as a template parameter 
so that it results in no overhead compared to a plain pointer.  
shared_ptr already has some overhead, so storing the deleter as a field 
of the control block isn't a big deal.  It makes it more flexible, 
however.  You can for example have a container of shared_ptr that have 
different deleters, which you can't do if the deleter is part of the 
type.

https://stackoverflow.com/questions/27742290/deleter-type-in-unique-ptr-vs-shared-ptr

This series LGTM.

Simon

Re: [RFA 3/3] Change counted_command_line to a shared_ptr

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> This series LGTM.

I somehow missed this approval
I'll check in the updated patches soon.

Tom