Re: [PATCH] Change bookmark allocation

[Simon Marchi <simark@simark.ca>]

On 2022-08-15 19:10, Tom Tromey wrote:
> This changes how bookmarks are allocated and stored, replacing a
> linked list with a vector and removing some ALL_* iterator macros.
> Regression tested on x86-64 Fedora 34.
> ---
>  gdb/reverse.c | 137 +++++++++++++++++---------------------------------
>  1 file changed, 46 insertions(+), 91 deletions(-)
> 
> diff --git a/gdb/reverse.c b/gdb/reverse.c
> index dc417497b7a..fc94b8d1825 100644
> --- a/gdb/reverse.c
> +++ b/gdb/reverse.c
> @@ -92,23 +92,15 @@ reverse_finish (const char *args, int from_tty)
>  /* Data structures for a bookmark list.  */
>  
>  struct bookmark {
> -  struct bookmark *next;
> -  int number;
> -  CORE_ADDR pc;
> -  struct symtab_and_line sal;
> -  gdb_byte *opaque_data;
> +  int number = 0;
> +  CORE_ADDR pc = 0;
> +  struct symtab_and_line sal {};

{} is unnecessary here, symtab_and_line already initializes its fields.

> +  gdb::unique_xmalloc_ptr<gdb_byte> opaque_data;
>  };
>  
> -static struct bookmark *bookmark_chain;
> +static std::vector<struct bookmark> all_bookmarks;
>  static int bookmark_count;
>  
> -#define ALL_BOOKMARKS(B) for ((B) = bookmark_chain; (B); (B) = (B)->next)
> -
> -#define ALL_BOOKMARKS_SAFE(B,TMP)           \
> -     for ((B) = bookmark_chain;             \
> -	  (B) ? ((TMP) = (B)->next, 1) : 0; \
> -	  (B) = (TMP))
> -
>  /* save_bookmark_command -- implement "bookmark" command.
>     Call target method to get a bookmark identifier.
>     Insert bookmark identifier into list.
> @@ -130,80 +122,46 @@ save_bookmark_command (const char *args, int from_tty)
>      error (_("target_get_bookmark failed."));
>  
>    /* Set up a bookmark struct.  */
> -  bookmark *b = new bookmark ();
> -  b->number = ++bookmark_count;
> -  b->pc = regcache_read_pc (get_current_regcache ());
> -  b->sal = find_pc_line (b->pc, 0);
> -  b->sal.pspace = get_frame_program_space (get_current_frame ());
> -  b->opaque_data = bookmark_id;
> -  b->next = NULL;
> -
> -  /* Add this bookmark to the end of the chain, so that a list
> -     of bookmarks will come out in order of increasing numbers.  */
> -
> -  bookmark *b1 = bookmark_chain;
> -  if (b1 == 0)
> -    bookmark_chain = b;
> -  else
> -    {
> -      while (b1->next)
> -	b1 = b1->next;
> -      b1->next = b;
> -    }
> -  gdb_printf (_("Saved bookmark %d at %s\n"), b->number,
> -	      paddress (gdbarch, b->sal.pc));
> +  bookmark &b = all_bookmarks.emplace_back ();

I don't think this works in C++11, emplace_back returns void.

Otherwise, LGTM.

Simon