More gdb/skip.c C++ification

  On 08/09/2017 07:31 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> That would avoid complicating the calling functions as well.
> Simon> Either way (adding std::moves or this) are fine for me, it's as
> Simon> you wish.
> 
> I've added the moves, and I'm checking it in.  Thanks.
> 

Thanks much for the fix, and sorry for not realizing I was
introducing the leak.  

I thought I had a local branch somewhere that C++ified skip.c
some more, and I spent a bit today looking for it, but couldn't
find it.  As penance for introducing the bug in the first place,
and since I was staring at skip.c, I ended up (re?)coding what I
thought I had, on top of Tom's patch.

It's a bit of churn, but there's nothing really complicated here.
It's just the next logical steps after Tom's patch.  See commit
log below.

WDYT?

Tested on GNU/Linux.

From 94ca8b042a5696e020d614be64f4bb83c1d111e6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 10 Aug 2017 19:58:24 +0100
Subject: [PATCH] More gdb/skip.c C++ification

- Make skiplist_entry a class with private data members.
- Move all construction logic to the ctor.
- Make skip_file_p etc be methods of skiplist_entry.
- Use std::list for the skip entries chain.  Make the list own its
  elements.
- Add a skiplist_entry_up typedef and use it more.
- Get rid of the ALL_SKIPLIST_ENTRIES/ALL_SKIPLIST_ENTRIES_SAFE
  macros, use range-for / iterators instead.
- function_name_is_marked_for_skip 'function_sal' argument must be
  non-NULL, so make it a reference instead.

All skiplist_entry invariants are now controlled by skiplist_entry
methods/internals.  Some gdb_asserts disappear for being redundant.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* infrun.c (process_event_stop_test): Adjust
	function_name_is_marked_for_skip call.
	* skip.c: Include <list>.
	(skiplist_entry): Make it a class with private fields, and
	getters/setters.
	(skiplist_entry_up): New typedef.
	(skiplist_entry_chain): Delete.
	(skiplist_entries): New.
	(skiplist_entry_count): Delete.
	(highest_skiplist_entry_num): New.
	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
	(add_skiplist_entry): Delete.
	(skiplist_entry::skiplist_entry): New.
	(skiplist_entry::add_entry): New.
	(skip_file_command, skip_function): Adjust.
	(compile_skip_regexp): Delete.
	(skip_command): Don't compile regexp here.  Adjust to use
	skiplist_entry::add_entry.
	(skip_info): Adjust to use range-for, skiplist_entry_up and getters.
	(skip_enable_command, skip_disable_command): Adjust to use
	range-for, skiplist_entry_up and setters.
	(skip_delete_command): Adjust to use std::list of
	skiplist_entry_up.
	(add_skiplist_entry): Delete.
	(skip_file_p): Delete, refactored as ...
	(skiplist_entry::do_skip_file_p): ... this new method.
	(skip_gfile_p): Delete, refactored as ...
	(skiplist_entry::do_gskip_file_p): ... this new method.
	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
	(skiplist_entry::skip_function_p): ... this new method.
	(function_name_is_marked_for_skip): Now returns bool, and takes
	the function sal by const reference.  Adjust to use range-for,
	skiplist_entry_up and skiplist_entry methods.
	(_initialize_step_skip): Remove references to
	skiplist_entry_chain, skiplist_entry_count.
	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
	takes the function sal by const reference.
---
 gdb/infrun.c |   2 +-
 gdb/skip.c   | 426 +++++++++++++++++++++++++++--------------------------------
 gdb/skip.h   |   8 +-
 3 files changed, 198 insertions(+), 238 deletions(-)
  

On 08/10/2017 08:34 PM, Pedro Alves wrote:
> On 08/09/2017 07:31 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>>
>> Simon> That would avoid complicating the calling functions as well.
>> Simon> Either way (adding std::moves or this) are fine for me, it's as
>> Simon> you wish.
>>
>> I've added the moves, and I'm checking it in.  Thanks.
>>
> 
> Thanks much for the fix, and sorry for not realizing I was
> introducing the leak.  
> 
> I thought I had a local branch somewhere that C++ified skip.c
> some more, and I spent a bit today looking for it, but couldn't
> find it.  As penance for introducing the bug in the first place,
> and since I was staring at skip.c, I ended up (re?)coding what I
> thought I had, on top of Tom's patch.
> 
> It's a bit of churn, but there's nothing really complicated here.
> It's just the next logical steps after Tom's patch.  See commit
> log below.
> 
> WDYT?
> 
> Tested on GNU/Linux.

Actually, now that I turn my automatic-brainless-C++ification-mode
switch off I can't really explain why I thought of making the entry
chain a std::list of pointers instead of a list of objects... (which
would spare the double allocation [node + element].)  I'll see about
changing that...

Thanks,
Pedro Alves

> 
> From 94ca8b042a5696e020d614be64f4bb83c1d111e6 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 10 Aug 2017 19:58:24 +0100
> Subject: [PATCH] More gdb/skip.c C++ification
> 
> - Make skiplist_entry a class with private data members.
> - Move all construction logic to the ctor.
> - Make skip_file_p etc be methods of skiplist_entry.
> - Use std::list for the skip entries chain.  Make the list own its
>   elements.
> - Add a skiplist_entry_up typedef and use it more.
> - Get rid of the ALL_SKIPLIST_ENTRIES/ALL_SKIPLIST_ENTRIES_SAFE
>   macros, use range-for / iterators instead.
> - function_name_is_marked_for_skip 'function_sal' argument must be
>   non-NULL, so make it a reference instead.
> 
> All skiplist_entry invariants are now controlled by skiplist_entry
> methods/internals.  Some gdb_asserts disappear for being redundant.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* infrun.c (process_event_stop_test): Adjust
> 	function_name_is_marked_for_skip call.
> 	* skip.c: Include <list>.
> 	(skiplist_entry): Make it a class with private fields, and
> 	getters/setters.
> 	(skiplist_entry_up): New typedef.
> 	(skiplist_entry_chain): Delete.
> 	(skiplist_entries): New.
> 	(skiplist_entry_count): Delete.
> 	(highest_skiplist_entry_num): New.
> 	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
> 	(add_skiplist_entry): Delete.
> 	(skiplist_entry::skiplist_entry): New.
> 	(skiplist_entry::add_entry): New.
> 	(skip_file_command, skip_function): Adjust.
> 	(compile_skip_regexp): Delete.
> 	(skip_command): Don't compile regexp here.  Adjust to use
> 	skiplist_entry::add_entry.
> 	(skip_info): Adjust to use range-for, skiplist_entry_up and getters.
> 	(skip_enable_command, skip_disable_command): Adjust to use
> 	range-for, skiplist_entry_up and setters.
> 	(skip_delete_command): Adjust to use std::list of
> 	skiplist_entry_up.
> 	(add_skiplist_entry): Delete.
> 	(skip_file_p): Delete, refactored as ...
> 	(skiplist_entry::do_skip_file_p): ... this new method.
> 	(skip_gfile_p): Delete, refactored as ...
> 	(skiplist_entry::do_gskip_file_p): ... this new method.
> 	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
> 	(skiplist_entry::skip_function_p): ... this new method.
> 	(function_name_is_marked_for_skip): Now returns bool, and takes
> 	the function sal by const reference.  Adjust to use range-for,
> 	skiplist_entry_up and skiplist_entry methods.
> 	(_initialize_step_skip): Remove references to
> 	skiplist_entry_chain, skiplist_entry_count.
> 	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
> 	takes the function sal by const reference.
> ---
>  gdb/infrun.c |   2 +-
>  gdb/skip.c   | 426 +++++++++++++++++++++++++++--------------------------------
>  gdb/skip.h   |   8 +-
>  3 files changed, 198 insertions(+), 238 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 8f966e2..d6723fd 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6788,7 +6788,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
>  	if (tmp_sal.line != 0
>  	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
> -						  &tmp_sal))
> +						  tmp_sal))
>  	  {
>  	    if (execution_direction == EXEC_REVERSE)
>  	      handle_step_into_function_backward (gdbarch, ecs);
> diff --git a/gdb/skip.c b/gdb/skip.c
> index 6ab6c91..b10f8fb 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -35,74 +35,122 @@
>  #include "fnmatch.h"
>  #include "gdb_regex.h"
>  #include "common/gdb_optional.h"
> +#include <list>
>  
> -struct skiplist_entry
> +class skiplist_entry
>  {
> -  skiplist_entry (bool file_is_glob_, std::string &&file_,
> -		  bool function_is_regexp_, std::string &&function_)
> -    : number (-1),
> -      file_is_glob (file_is_glob_),
> -      file (std::move (file_)),
> -      function_is_regexp (function_is_regexp_),
> -      function (std::move (function_)),
> -      enabled (true),
> -      next (NULL)
> -  {
> -  }
> -
> -  int number;
> +public:
> +  /* Create a skiplist_entry object and add it to the chain.  */
> +  static void add_entry (bool file_is_glob,
> +			 std::string &&file,
> +			 bool function_is_regexp,
> +			 std::string &&function);
> +
> +  /* Return true if the skip entry has a file or glob-style file
> +     pattern that matches FUNCTION_SAL.  */
> +  bool skip_file_p (const symtab_and_line &function_sal);
> +
> +  /* Return true if the skip entry has a function or function regexp
> +     that matches FUNCTION_NAME.  */
> +  bool skip_function_p (const char *function_name);
> +
> +  /* Getters.  */
> +  int number () const { return m_number; };
> +  bool enabled () const { return m_enabled; };
> +  bool file_is_glob () const { return m_file_is_glob; }
> +  const std::string &file () const { return m_file; }
> +  const std::string &function () const { return m_function; }
> +  bool function_is_regexp () const { return m_function_is_regexp; }
> +
> +  /* Setters.  */
> +  void enable () { m_enabled = true; };
> +  void disable () { m_enabled = false; };
> +
> +private:
> +  /* Use add_entry factory to construct instead.  */
> +  skiplist_entry (bool file_is_glob, std::string &&file,
> +		  bool function_is_regexp, std::string &&function);
> +
> +  /* Return true if we're stopped at a file to be skipped.  */
> +  bool do_skip_file_p (const symtab_and_line &function_sal);
> +
> +  /* Return true if we're stopped at a globbed file to be skipped.  */
> +  bool do_skip_gfile_p (const symtab_and_line &function_sal);
> +
> +private: /* data */
> +  int m_number = -1;
>  
>    /* True if FILE is a glob-style pattern.
>       Otherwise it is the plain file name (possibly with directories).  */
> -  bool file_is_glob;
> +  bool m_file_is_glob;
>  
>    /* The name of the file or empty if no name.  */
> -  std::string file;
> +  std::string m_file;
>  
>    /* True if FUNCTION is a regexp.
>       Otherwise it is a plain function name (possibly with arguments,
>       for C++).  */
> -  bool function_is_regexp;
> +  bool m_function_is_regexp;
>  
>    /* The name of the function or empty if no name.  */
> -  std::string function;
> +  std::string m_function;
>  
>    /* If this is a function regexp, the compiled form.  */
> -  gdb::optional<compiled_regex> compiled_function_regexp;
> +  gdb::optional<compiled_regex> m_compiled_function_regexp;
>  
> -  bool enabled;
> -
> -  struct skiplist_entry *next;
> +  /* Enabled/disabled state.  */
> +  bool m_enabled = true;
>  };
>  
> -static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
> +typedef std::unique_ptr<skiplist_entry> skiplist_entry_up;
>  
> -static struct skiplist_entry *skiplist_entry_chain;
> -static int skiplist_entry_count;
> +static std::list<skiplist_entry_up> skiplist_entries;
> +static int highest_skiplist_entry_num = 0;
>  
> -#define ALL_SKIPLIST_ENTRIES(E) \
> -  for (E = skiplist_entry_chain; E; E = E->next)
> +skiplist_entry::skiplist_entry (bool file_is_glob,
> +				std::string &&file,
> +				bool function_is_regexp,
> +				std::string &&function)
> +  : m_file_is_glob (file_is_glob),
> +    m_file (std::move (file)),
> +    m_function_is_regexp (function_is_regexp),
> +    m_function (std::move (function))
> +{
> +  gdb_assert (!m_file.empty () || !m_function.empty ());
>  
> -#define ALL_SKIPLIST_ENTRIES_SAFE(E,TMP) \
> -  for (E = skiplist_entry_chain;         \
> -       E ? (TMP = E->next, 1) : 0;       \
> -       E = TMP)
> +  if (m_file_is_glob)
> +    gdb_assert (!m_file.empty ());
> +
> +  if (m_function_is_regexp)
> +    {
> +      gdb_assert (!m_function.empty ());
> +
> +      int flags = REG_NOSUB;
> +#ifdef REG_EXTENDED
> +      flags |= REG_EXTENDED;
> +#endif
>  
> -/* Create a skip object.  */
> +      gdb_assert (!m_function.empty ());
> +      m_compiled_function_regexp.emplace (m_function.c_str (), flags,
> +					  _("regexp"));
> +    }
> +}
>  
> -static std::unique_ptr<skiplist_entry>
> -make_skip_entry (bool file_is_glob, std::string &&file,
> -		 bool function_is_regexp, std::string &&function)
> +void
> +skiplist_entry::add_entry (bool file_is_glob, std::string &&file,
> +			   bool function_is_regexp, std::string &&function)
>  {
> -  gdb_assert (!file.empty () || !function.empty ());
> -  if (file_is_glob)
> -    gdb_assert (!file.empty ());
> -  if (function_is_regexp)
> -    gdb_assert (!function.empty ());
> -
> -  return std::unique_ptr<skiplist_entry>
> -    (new skiplist_entry (file_is_glob, std::move (file),
> -			 function_is_regexp, std::move (function)));
> +  skiplist_entry *e = new skiplist_entry (file_is_glob,
> +					  std::move (file),
> +					  function_is_regexp,
> +					  std::move (function));
> +
> +  /* Add to the end of the chain so that the list of skiplist entries
> +     is always in numerical order.  */
> +  skiplist_entries.push_back (skiplist_entry_up (e));
> +
> +  /* Incremented after push_back, in case push_back throws.  */
> +  e->m_number = ++highest_skiplist_entry_num;
>  }
>  
>  static void
> @@ -126,8 +174,8 @@ skip_file_command (char *arg, int from_tty)
>    else
>      filename = arg;
>  
> -  add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
> -				       std::string ()));
> +  skiplist_entry::add_entry (false, std::string (filename),
> +			     false, std::string ());
>  
>    printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
>  }
> @@ -138,8 +186,7 @@ skip_file_command (char *arg, int from_tty)
>  static void
>  skip_function (const char *name)
>  {
> -  add_skiplist_entry (make_skip_entry (false, std::string (), false,
> -				       std::string (name)));
> +  skiplist_entry::add_entry (false, std::string (), false, std::string (name));
>  
>    printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
>  }
> @@ -169,23 +216,6 @@ skip_function_command (char *arg, int from_tty)
>    skip_function (arg);
>  }
>  
> -/* Compile the regexp in E.
> -   An error is thrown if there's an error.
> -   MESSAGE is used as a prefix of the error message.  */
> -
> -static void
> -compile_skip_regexp (struct skiplist_entry *e, const char *message)
> -{
> -  int flags = REG_NOSUB;
> -
> -#ifdef REG_EXTENDED
> -  flags |= REG_EXTENDED;
> -#endif
> -
> -  gdb_assert (e->function_is_regexp && !e->function.empty ());
> -  e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
> -}
> -
>  /* Process "skip ..." that does not match "skip file" or "skip function".  */
>  
>  static void
> @@ -273,18 +303,15 @@ skip_command (char *arg, int from_tty)
>      entry_file = file;
>    else if (gfile != NULL)
>      entry_file = gfile;
> +
>    std::string entry_function;
>    if (function != NULL)
>      entry_function = function;
>    else if (rfunction != NULL)
>      entry_function = rfunction;
> -  std::unique_ptr<skiplist_entry> e
> -    = make_skip_entry (gfile != NULL, std::move (entry_file),
> -		       rfunction != NULL, std::move (entry_function));
> -  if (rfunction != NULL)
> -    compile_skip_regexp (e.get (), _("regexp"));
>  
> -  add_skiplist_entry (std::move (e));
> +  skiplist_entry::add_entry (gfile != NULL, std::move (entry_file),
> +			     rfunction != NULL, std::move (entry_function));
>  
>    /* I18N concerns drive some of the choices here (we can't piece together
>       the output too much).  OTOH we want to keep this simple.  Therefore the
> @@ -321,7 +348,6 @@ skip_command (char *arg, int from_tty)
>  static void
>  skip_info (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
>    int num_printable_entries = 0;
>    struct value_print_options opts;
>  
> @@ -329,8 +355,8 @@ skip_info (char *arg, int from_tty)
>  
>    /* Count the number of rows in the table and see if we need space for a
>       64-bit address anywhere.  */
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        num_printable_entries++;
>  
>    if (num_printable_entries == 0)
> @@ -355,37 +381,36 @@ skip_info (char *arg, int from_tty)
>    current_uiout->table_header (40, ui_noalign, "function", "Function"); /* 6 */
>    current_uiout->table_body ();
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> +  for (const skiplist_entry_up &e : skiplist_entries)
>      {
> -
>        QUIT;
> -      if (arg != NULL && !number_is_in_list (arg, e->number))
> +      if (arg != NULL && !number_is_in_list (arg, e->number ()))
>  	continue;
>  
>        ui_out_emit_tuple tuple_emitter (current_uiout, "blklst-entry");
> -      current_uiout->field_int ("number", e->number); /* 1 */
> +      current_uiout->field_int ("number", e->number ()); /* 1 */
>  
> -      if (e->enabled)
> +      if (e->enabled ())
>  	current_uiout->field_string ("enabled", "y"); /* 2 */
>        else
>  	current_uiout->field_string ("enabled", "n"); /* 2 */
>  
> -      if (e->file_is_glob)
> +      if (e->file_is_glob ())
>  	current_uiout->field_string ("regexp", "y"); /* 3 */
>        else
>  	current_uiout->field_string ("regexp", "n"); /* 3 */
>  
>        current_uiout->field_string ("file",
> -				   e->file.empty () ? "<none>"
> -				   : e->file.c_str ()); /* 4 */
> -      if (e->function_is_regexp)
> +				   e->file ().empty () ? "<none>"
> +				   : e->file ().c_str ()); /* 4 */
> +      if (e->function_is_regexp ())
>  	current_uiout->field_string ("regexp", "y"); /* 5 */
>        else
>  	current_uiout->field_string ("regexp", "n"); /* 5 */
>  
>        current_uiout->field_string ("function",
> -				   e->function.empty () ? "<none>"
> -				   : e->function.c_str ()); /* 6 */
> +				   e->function ().empty () ? "<none>"
> +				   : e->function ().c_str ()); /* 6 */
>  
>        current_uiout->text ("\n");
>      }
> @@ -394,14 +419,13 @@ skip_info (char *arg, int from_tty)
>  static void
>  skip_enable_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
> -  int found = 0;
> +  bool found = false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        {
> -        e->enabled = true;
> -        found = 1;
> +	e->enable ();
> +	found = true;
>        }
>  
>    if (!found)
> @@ -411,14 +435,13 @@ skip_enable_command (char *arg, int from_tty)
>  static void
>  skip_disable_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
> -  int found = 0;
> +  bool found = false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        {
> -	e->enabled = false;
> -        found = 1;
> +	e->disable ();
> +	found = true;
>        }
>  
>    if (!found)
> @@ -428,104 +451,62 @@ skip_disable_command (char *arg, int from_tty)
>  static void
>  skip_delete_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e, *temp, *b_prev;
> -  int found = 0;
> +  bool found = false;
>  
> -  b_prev = 0;
> -  ALL_SKIPLIST_ENTRIES_SAFE (e, temp)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> -      {
> -	if (b_prev != NULL)
> -	  b_prev->next = e->next;
> -	else
> -	  skiplist_entry_chain = e->next;
> +  for (auto it = skiplist_entries.begin (),
> +	 end = skiplist_entries.end ();
> +       it != end;)
> +    {
> +      const skiplist_entry_up &e = *it;
>  
> -	delete e;
> -        found = 1;
> -      }
> -    else
> -      {
> -	b_prev = e;
> -      }
> +      if (arg == NULL || number_is_in_list (arg, e->number ()))
> +	{
> +	  it = skiplist_entries.erase (it);
> +	  found = true;
> +	}
> +      else
> +	++it;
> +    }
>  
>    if (!found)
>      error (_("No skiplist entries found with number %s."), arg);
>  }
>  
> -/* Add the given skiplist entry to our list, and set the entry's number.  */
> -
> -static void
> -add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
> +bool
> +skiplist_entry::do_skip_file_p (const symtab_and_line &function_sal)
>  {
> -  struct skiplist_entry *e1;
> -
> -  e->number = ++skiplist_entry_count;
> -
> -  /* Add to the end of the chain so that the list of
> -     skiplist entries will be in numerical order.  */
> -
> -  e1 = skiplist_entry_chain;
> -  if (e1 == NULL)
> -    skiplist_entry_chain = e.release ();
> -  else
> -    {
> -      while (e1->next)
> -	e1 = e1->next;
> -      e1->next = e.release ();
> -    }
> -}
> -
> -/* Return non-zero if we're stopped at a file to be skipped.  */
> -
> -static int
> -skip_file_p (struct skiplist_entry *e,
> -	     const struct symtab_and_line *function_sal)
> -{
> -  gdb_assert (!e->file.empty () && !e->file_is_glob);
> -
> -  if (function_sal->symtab == NULL)
> -    return 0;
> -
>    /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
>       symtab_to_fullname as it may contain "./" etc.  */
> -  if (compare_filenames_for_search (function_sal->symtab->filename,
> -				    e->file.c_str ()))
> -    return 1;
> +  if (compare_filenames_for_search (function_sal.symtab->filename,
> +				    m_file.c_str ()))
> +    return true;
>  
>    /* Before we invoke realpath, which can get expensive when many
>       files are involved, do a quick comparison of the basenames.  */
>    if (!basenames_may_differ
> -      && filename_cmp (lbasename (function_sal->symtab->filename),
> -		       lbasename (e->file.c_str ())) != 0)
> -    return 0;
> +      && filename_cmp (lbasename (function_sal.symtab->filename),
> +		       lbasename (m_file.c_str ())) != 0)
> +    return false;
>  
>    /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
>    {
> -    const char *fullname = symtab_to_fullname (function_sal->symtab);
> +    const char *fullname = symtab_to_fullname (function_sal.symtab);
>  
> -    if (compare_filenames_for_search (fullname, e->file.c_str ()))
> -      return 1;
> +    if (compare_filenames_for_search (fullname, m_file.c_str ()))
> +      return true;
>    }
>  
> -  return 0;
> +  return false;
>  }
>  
> -/* Return non-zero if we're stopped at a globbed file to be skipped.  */
> -
> -static int
> -skip_gfile_p (struct skiplist_entry *e,
> -	      const struct symtab_and_line *function_sal)
> +bool
> +skiplist_entry::do_skip_gfile_p (const symtab_and_line &function_sal)
>  {
> -  gdb_assert (!e->file.empty () && e->file_is_glob);
> -
> -  if (function_sal->symtab == NULL)
> -    return 0;
> -
>    /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
>       symtab_to_fullname as it may contain "./" etc.  */
> -  if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
> +  if (gdb_filename_fnmatch (m_file.c_str (), function_sal.symtab->filename,
>  			    FNM_FILE_NAME | FNM_NOESCAPE) == 0)
> -    return 1;
> +    return true;
>  
>    /* Before we invoke symtab_to_fullname, which is expensive, do a quick
>       comparison of the basenames.
> @@ -533,101 +514,83 @@ skip_gfile_p (struct skiplist_entry *e,
>       If the basename of the glob pattern is something like "*.c" then this
>       isn't much of a win.  Oh well.  */
>    if (!basenames_may_differ
> -      && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
> -			       lbasename (function_sal->symtab->filename),
> +      && gdb_filename_fnmatch (lbasename (m_file.c_str ()),
> +			       lbasename (function_sal.symtab->filename),
>  			       FNM_FILE_NAME | FNM_NOESCAPE) != 0)
> -    return 0;
> +    return false;
>  
>    /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
>    {
> -    const char *fullname = symtab_to_fullname (function_sal->symtab);
> +    const char *fullname = symtab_to_fullname (function_sal.symtab);
>  
> -    if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
> -      return 1;
> +    if (compare_glob_filenames_for_search (fullname, m_file.c_str ()))
> +      return true;
>    }
>  
> -  return 0;
> +  return false;
>  }
>  
> -/* Return non-zero if we're stopped at a function to be skipped.  */
> -
> -static int
> -skip_function_p (struct skiplist_entry *e, const char *function_name)
> +bool
> +skiplist_entry::skip_file_p (const symtab_and_line &function_sal)
>  {
> -  gdb_assert (!e->function.empty () && !e->function_is_regexp);
> -  return strcmp_iw (function_name, e->function.c_str ()) == 0;
> -}
> +  if (m_file.empty ())
> +    return false;
>  
> -/* Return non-zero if we're stopped at a function regexp to be skipped.  */
> +  if (function_sal.symtab == NULL)
> +    return false;
>  
> -static int
> -skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
> +  if (m_file_is_glob)
> +    return do_skip_gfile_p (function_sal);
> +  else
> +    return do_skip_file_p (function_sal);
> +}
> +
> +bool
> +skiplist_entry::skip_function_p (const char *function_name)
>  {
> -  gdb_assert (!e->function.empty () && e->function_is_regexp
> -	      && e->compiled_function_regexp);
> -  return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
> -	  == 0);
> +  if (m_function.empty ())
> +    return false;
> +
> +  if (m_function_is_regexp)
> +    {
> +      gdb_assert (m_compiled_function_regexp);
> +      return (m_compiled_function_regexp->exec (function_name, 0, NULL, 0)
> +	      == 0);
> +    }
> +  else
> +    return strcmp_iw (function_name, m_function.c_str ()) == 0;
>  }
>  
>  /* See skip.h.  */
>  
> -int
> +bool
>  function_name_is_marked_for_skip (const char *function_name,
> -				  const struct symtab_and_line *function_sal)
> +				  const symtab_and_line &function_sal)
>  {
> -  struct skiplist_entry *e;
> -
>    if (function_name == NULL)
> -    return 0;
> +    return false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> +  for (const skiplist_entry_up &e : skiplist_entries)
>      {
> -      int skip_by_file = 0;
> -      int skip_by_function = 0;
> -
> -      if (!e->enabled)
> +      if (!e->enabled ())
>  	continue;
>  
> -      if (!e->file.empty ())
> -	{
> -	  if (e->file_is_glob)
> -	    {
> -	      if (skip_gfile_p (e, function_sal))
> -		skip_by_file = 1;
> -	    }
> -	  else
> -	    {
> -	      if (skip_file_p (e, function_sal))
> -		skip_by_file = 1;
> -	    }
> -	}
> -      if (!e->function.empty ())
> -	{
> -	  if (e->function_is_regexp)
> -	    {
> -	      if (skip_rfunction_p (e, function_name))
> -		skip_by_function = 1;
> -	    }
> -	  else
> -	    {
> -	      if (skip_function_p (e, function_name))
> -		skip_by_function = 1;
> -	    }
> -	}
> +      bool skip_by_file = e->skip_file_p (function_sal);
> +      bool skip_by_function = e->skip_function_p (function_name);
>  
>        /* If both file and function must match, make sure we don't errantly
>  	 exit if only one of them match.  */
> -      if (!e->file.empty () && !e->function.empty ())
> +      if (!e->file ().empty () && !e->function ().empty ())
>  	{
>  	  if (skip_by_file && skip_by_function)
> -	    return 1;
> +	    return true;
>  	}
>        /* Only one of file/function is specified.  */
>        else if (skip_by_file || skip_by_function)
> -	return 1;
> +	return true;
>      }
>  
> -  return 0;
> +  return false;
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> @@ -639,9 +602,6 @@ _initialize_step_skip (void)
>    static struct cmd_list_element *skiplist = NULL;
>    struct cmd_list_element *c;
>  
> -  skiplist_entry_chain = 0;
> -  skiplist_entry_count = 0;
> -
>    add_prefix_cmd ("skip", class_breakpoint, skip_command, _("\
>  Ignore a function while stepping.\n\
>  \n\
> diff --git a/gdb/skip.h b/gdb/skip.h
> index dbc92d8..4e1b544 100644
> --- a/gdb/skip.h
> +++ b/gdb/skip.h
> @@ -20,9 +20,9 @@
>  
>  struct symtab_and_line;
>  
> -/* Returns 1 if the given FUNCTION_NAME is marked for skip and shouldn't be
> -   stepped into.  Otherwise, returns 0.  */
> -int function_name_is_marked_for_skip (const char *function_name,
> -				    const struct symtab_and_line *function_sal);
> +/* Returns true if the given FUNCTION_NAME is marked for skip and
> +   shouldn't be stepped into.  Otherwise, returns false.  */
> +bool function_name_is_marked_for_skip (const char *function_name,
> +				       const symtab_and_line &function_sal);
>  
>  #endif /* !defined (SKIP_H) */
>
  

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

Pedro> It's a bit of churn, but there's nothing really complicated here.
Pedro> It's just the next logical steps after Tom's patch.  See commit
Pedro> log below.

Pedro> WDYT?

It all seems good to me (though I only skimmed it).

Tom