[2/4] gdb: make struct output_source_filename_data more C++ like

Message ID b301ce462403c3cac6d00488fa8eef00192f03b2.1619456691.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • New option for 'info sources', also better MI support
Related show

Commit Message

Andrew Burgess April 26, 2021, 5:07 p.m.
In a future commit I'm going to be making some changes to the 'info
sources' command.  While looking at the code I noticed that things
could be improved by making struct output_source_filename_data more
C++ like (private member variables, and more member functions).
That's what this commit does.

The change to make filename_partial_match_opts private within
output_source_filename_data might seem unhelpful (we now have to
create a stack local variable and initialise this field through the
constructor), however, in a later commit I plan to split the function
info_sources_command in two, and having output_source_filename_data
initialised entirely through the constructor will make this process
easier.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* symtab.c (struct filename_partial_match_opts): Update comment.
	(struct output_source_filename_data)
	<output_source_filename_data>: New constructor.  <regexp>: Rename
	to m_regexp.  <c_regexp>: Rename to m_c_regexp.  <reset_output>:
	New member function.  <filename_seen_cache>: Rename to
	m_filename_seen_cache, change from being a pointer, to being an
	actual object.  <set_regexp>: New member function.  <first>:
	Rename to m_first.  <print_header>: New member
	function. <partial_match>: Renamed to m_partial_match.
	(output_source_filename_data::output): Update now
	m_filename_seen_cache is no longer a pointer, and for other member
	variable name changes. Add a header comment.
	(print_info_sources_header): Renamed to...
	(output_source_filename_data::print_header): ...this.  Update now
	it's a member function and to take account of member variable
	renaming.
	(info_sources_command): Add a header comment, delete stack local
	filename_seen_cache, initialization of output_source_filename_data
	is now done by the constructor.  Call print_header member function
	instead of print_info_sources_header, call reset_output member
	function instead of manually performing the reset.
---
 gdb/ChangeLog |  24 +++++++
 gdb/symtab.c  | 183 +++++++++++++++++++++++++++++---------------------
 2 files changed, 129 insertions(+), 78 deletions(-)

-- 
2.25.4

Comments

Carl Love via Gdb-patches May 13, 2021, 2:58 p.m. | #1
LGTM, I just noted a few nits / potential future cleanups.

> @@ -4211,35 +4212,83 @@ struct filename_partial_match_opts

>    bool basename = false;

>  };

>  

> -/* Data structure to maintain printing state for output_source_filename.  */

> +/* Data structure to maintain the state used for printing the results of

> +   the 'info sources' command.  */

>  

>  struct output_source_filename_data

>  {

> -  /* Output only filenames matching REGEXP.  */

> -  std::string regexp;

> -  gdb::optional<compiled_regex> c_regexp;

> -  /* Possibly only match a part of the filename.  */

> -  filename_partial_match_opts partial_match;

> +  output_source_filename_data (const char *regexp,

> +			       bool match_on_dirname,

> +			       bool match_on_basename)

> +  {

> +    m_partial_match.dirname = match_on_dirname;

> +    m_partial_match.basename = match_on_basename;


You could initialize in in the initializer list:

  : m_partial_match {match_on_dirname, match_on_basename}

>  

> +private:

> +

> +  /* Set the current regular expression, used for matching against files,

> +     to REGEXP.  */

> +  void set_regexp (const std::string regexp)


I'm a bit surprised that this is "const", but gets moved.  I'm
surprised this even compiles at all!

> +  {

> +    m_regexp = std::move (regexp);

> +    if (!m_regexp.empty ())

> +      {

> +	int cflags = REG_NOSUB;

> +#ifdef HAVE_CASE_INSENSITIVE_FILE_SYSTEM

> +	cflags |= REG_ICASE;

> +#endif

> +	m_c_regexp.emplace (m_regexp.c_str (), cflags,

> +			  _("Invalid regexp"));


This last line is missing a bit of indentation.

> +      }

> +  }

> +

> +  /* Output only filenames matching REGEXP.  */

> +  std::string m_regexp;

> +  gdb::optional<compiled_regex> m_c_regexp;

> +

> +  /* Flag of whether we're printing the first one.  */

> +  bool m_first = true;

> +

> +  /* Cache of what we've seen so far.  */

> +  filename_seen_cache m_filename_seen_cache;

> +

> +  /* Possibly only match a part of the filename.  */

> +  filename_partial_match_opts m_partial_match;

>  };

>  

> +/* See comment is class declaration above.  */


is -> in

I'm all for adding comments to functions, but I feel that these comments
(the /* See foo.h.  */ typically) are a bit useless.  If our standard is
to document external functions in headers and class declarations, do we
really need a comment to tell us to go look there for us to go look
there? >/morning rant>

> @@ -4354,49 +4399,33 @@ info_sources_command_completer (cmd_list_element *ignore,

>      return;

>  }

>  

> +/* Implement the 'info sources' command.  */

> +

>  static void

>  info_sources_command (const char *args, int from_tty)

>  {

> -  struct output_source_filename_data data;

> -

>    if (!have_full_symbols () && !have_partial_symbols ())

> -    {

> -      error (_("No symbol table is loaded.  Use the \"file\" command."));

> -    }

> -

> -  filename_seen_cache filenames_seen;

> -

> -  auto group = make_info_sources_options_def_group (&data.partial_match);

> +    error (_("No symbol table is loaded.  Use the \"file\" command."));

>  

> +  struct filename_partial_match_opts match_opts;

> +  auto group = make_info_sources_options_def_group (&match_opts);

>    gdb::option::process_options

>      (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group);

>  

> -  if (args != NULL && *args != '\000')

> -    data.regexp = args;

> +  if (match_opts.dirname && match_opts.basename)

> +    error (_("You cannot give both -basename and -dirname to 'info sources'."));


That's orthogonal to your change, but given that match_on_dirname and
match_on_basename are mutually exclusive, I think that
output_source_filename_data / filename_partial_match_opts should take an
enum instead, like

  enum class match_what
  {
    all,
    dirname,
    basename,
  };

It would be clearer that it's one or the other.  And it would by design
make it impossible for it to have an invalid state (both booleans to
true).

Simon

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 061177e1d23..5650d225752 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4200,7 +4200,8 @@  operator_chars (const char *p, const char **end)
 }
 
 
-/* What part to match in a file name.  */
+/* For the 'info sources' command, what part of the file names should we be
+   matching the user supplied regular expression against?  */
 
 struct filename_partial_match_opts
 {
@@ -4211,35 +4212,83 @@  struct filename_partial_match_opts
   bool basename = false;
 };
 
-/* Data structure to maintain printing state for output_source_filename.  */
+/* Data structure to maintain the state used for printing the results of
+   the 'info sources' command.  */
 
 struct output_source_filename_data
 {
-  /* Output only filenames matching REGEXP.  */
-  std::string regexp;
-  gdb::optional<compiled_regex> c_regexp;
-  /* Possibly only match a part of the filename.  */
-  filename_partial_match_opts partial_match;
+  output_source_filename_data (const char *regexp,
+			       bool match_on_dirname,
+			       bool match_on_basename)
+  {
+    m_partial_match.dirname = match_on_dirname;
+    m_partial_match.basename = match_on_basename;
 
+    if (regexp != nullptr)
+      set_regexp (regexp);
+  }
 
-  /* Cache of what we've seen so far.  */
-  struct filename_seen_cache *filename_seen_cache;
+  DISABLE_COPY_AND_ASSIGN (output_source_filename_data);
 
-  /* Flag of whether we're printing the first one.  */
-  int first;
+  /* Reset enough state of this object so we can match against a new set of
+     files.  The existing regular expression is retained though.  */
+  void reset_output ()
+  {
+    m_first = true;
+    m_filename_seen_cache.clear ();
+  }
 
   /* Worker for sources_info.  Force line breaks at ,'s.
      NAME is the name to print.  */
   void output (const char *name);
 
+  /* Prints the header messages for the source files that will be printed
+     with the matching info present in the current object state.
+     SYMBOL_MSG is a message that describes what will or has been done with
+     the symbols of the matching source files.  */
+  void print_header (const char *symbol_msg);
+
   /* An overload suitable for use as a callback to
      quick_symbol_functions::map_symbol_filenames.  */
   void operator() (const char *filename, const char *fullname)
   {
     output (fullname != nullptr ? fullname : filename);
   }
+
+private:
+
+  /* Set the current regular expression, used for matching against files,
+     to REGEXP.  */
+  void set_regexp (const std::string regexp)
+  {
+    m_regexp = std::move (regexp);
+    if (!m_regexp.empty ())
+      {
+	int cflags = REG_NOSUB;
+#ifdef HAVE_CASE_INSENSITIVE_FILE_SYSTEM
+	cflags |= REG_ICASE;
+#endif
+	m_c_regexp.emplace (m_regexp.c_str (), cflags,
+			  _("Invalid regexp"));
+      }
+  }
+
+  /* Output only filenames matching REGEXP.  */
+  std::string m_regexp;
+  gdb::optional<compiled_regex> m_c_regexp;
+
+  /* Flag of whether we're printing the first one.  */
+  bool m_first = true;
+
+  /* Cache of what we've seen so far.  */
+  filename_seen_cache m_filename_seen_cache;
+
+  /* Possibly only match a part of the filename.  */
+  filename_partial_match_opts m_partial_match;
 };
 
+/* See comment is class declaration above.  */
+
 void
 output_source_filename_data::output (const char *name)
 {
@@ -4253,41 +4302,62 @@  output_source_filename_data::output (const char *name)
      symtabs; it doesn't hurt to check.  */
 
   /* Was NAME already seen?  */
-  if (filename_seen_cache->seen (name))
+  if (m_filename_seen_cache.seen (name))
     {
       /* Yes; don't print it again.  */
       return;
     }
 
   /* Does it match regexp?  */
-  if (c_regexp.has_value ())
+  if (m_c_regexp.has_value ())
     {
       const char *to_match;
       std::string dirname;
 
-      if (partial_match.dirname)
+      if (m_partial_match.dirname)
 	{
 	  dirname = ldirname (name);
 	  to_match = dirname.c_str ();
 	}
-      else if (partial_match.basename)
+      else if (m_partial_match.basename)
 	to_match = lbasename (name);
       else
 	to_match = name;
 
-      if (c_regexp->exec (to_match, 0, NULL, 0) != 0)
+      if (m_c_regexp->exec (to_match, 0, NULL, 0) != 0)
 	return;
     }
 
   /* Print it and reset *FIRST.  */
-  if (! first)
+  if (!m_first)
     printf_filtered (", ");
-  first = 0;
+  m_first = false;
 
   wrap_here ("");
   fputs_styled (name, file_name_style.style (), gdb_stdout);
 }
 
+/* See comment is class declaration above.  */
+
+void
+output_source_filename_data::print_header (const char *symbol_msg)
+{
+  puts_filtered (symbol_msg);
+  if (!m_regexp.empty ())
+    {
+      if (m_partial_match.dirname)
+	printf_filtered (_("(dirname matching regular expression \"%s\")"),
+			 m_regexp.c_str ());
+      else if (m_partial_match.basename)
+	printf_filtered (_("(basename matching regular expression \"%s\")"),
+			 m_regexp.c_str ());
+      else
+	printf_filtered (_("(filename matching regular expression \"%s\")"),
+			 m_regexp.c_str ());
+    }
+  puts_filtered ("\n");
+}
+
 using isrc_flag_option_def
   = gdb::option::flag_option_def<filename_partial_match_opts>;
 
@@ -4316,31 +4386,6 @@  make_info_sources_options_def_group (filename_partial_match_opts *isrc_opts)
   return {{info_sources_option_defs}, isrc_opts};
 }
 
-/* Prints the header message for the source files that will be printed
-   with the matching info present in DATA.  SYMBOL_MSG is a message
-   that tells what will or has been done with the symbols of the
-   matching source files.  */
-
-static void
-print_info_sources_header (const char *symbol_msg,
-			   const struct output_source_filename_data *data)
-{
-  puts_filtered (symbol_msg);
-  if (!data->regexp.empty ())
-    {
-      if (data->partial_match.dirname)
-	printf_filtered (_("(dirname matching regular expression \"%s\")"),
-			 data->regexp.c_str ());
-      else if (data->partial_match.basename)
-	printf_filtered (_("(basename matching regular expression \"%s\")"),
-			 data->regexp.c_str ());
-      else
-	printf_filtered (_("(filename matching regular expression \"%s\")"),
-			 data->regexp.c_str ());
-    }
-  puts_filtered ("\n");
-}
-
 /* Completer for "info sources".  */
 
 static void
@@ -4354,49 +4399,33 @@  info_sources_command_completer (cmd_list_element *ignore,
     return;
 }
 
+/* Implement the 'info sources' command.  */
+
 static void
 info_sources_command (const char *args, int from_tty)
 {
-  struct output_source_filename_data data;
-
   if (!have_full_symbols () && !have_partial_symbols ())
-    {
-      error (_("No symbol table is loaded.  Use the \"file\" command."));
-    }
-
-  filename_seen_cache filenames_seen;
-
-  auto group = make_info_sources_options_def_group (&data.partial_match);
+    error (_("No symbol table is loaded.  Use the \"file\" command."));
 
+  struct filename_partial_match_opts match_opts;
+  auto group = make_info_sources_options_def_group (&match_opts);
   gdb::option::process_options
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, group);
 
-  if (args != NULL && *args != '\000')
-    data.regexp = args;
+  if (match_opts.dirname && match_opts.basename)
+    error (_("You cannot give both -basename and -dirname to 'info sources'."));
 
-  data.filename_seen_cache = &filenames_seen;
-  data.first = 1;
+  const char *regex = nullptr;
+  if (args != nullptr && *args != '\000')
+    regex = args;
 
-  if (data.partial_match.dirname && data.partial_match.basename)
-    error (_("You cannot give both -basename and -dirname to 'info sources'."));
-  if ((data.partial_match.dirname || data.partial_match.basename)
-      && data.regexp.empty ())
-     error (_("Missing REGEXP for 'info sources'."));
+  if ((match_opts.dirname || match_opts.basename) && regex == nullptr)
+    error (_("Missing REGEXP for 'info sources'."));
 
-  if (data.regexp.empty ())
-    data.c_regexp.reset ();
-  else
-    {
-      int cflags = REG_NOSUB;
-#ifdef HAVE_CASE_INSENSITIVE_FILE_SYSTEM
-      cflags |= REG_ICASE;
-#endif
-      data.c_regexp.emplace (data.regexp.c_str (), cflags,
-			     _("Invalid regexp"));
-    }
+  struct output_source_filename_data data (regex, match_opts.dirname,
+					   match_opts.basename);
 
-  print_info_sources_header
-    (_("Source files for which symbols have been read in:\n"), &data);
+  data.print_header (_("Source files for which symbols have been read in:\n"));
 
   for (objfile *objfile : current_program_space->objfiles ())
     {
@@ -4412,11 +4441,9 @@  info_sources_command (const char *args, int from_tty)
     }
   printf_filtered ("\n\n");
 
-  print_info_sources_header
-    (_("Source files for which symbols will be read in on demand:\n"), &data);
+  data.print_header (_("Source files for which symbols will be read in on demand:\n"));
 
-  filenames_seen.clear ();
-  data.first = 1;
+  data.reset_output ();
   map_symbol_filenames (data, true /*need_fullname*/);
   printf_filtered ("\n");
 }