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

[Simon Marchi <simon.marchi@polymtl.ca>]

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