Re: [PATCH v2 1/3] Convert generic probe interface to C++ (and perform some cleanups)

[Simon Marchi <simon.marchi@ericsson.com>]

On 2017-11-15 11:37 PM, Sergio Durigan Junior wrote:
> Changes from v1:
> 
> - Remove emit_info_probes_extra_fields method.
> 
> - Make gen_info_* methods return the vector directly.  Modify code
>   accordingly.
> 
> - Cleanup unused variables.
> 
> - Make can_evaluate_arguments return bool.
> 
> - Make get_name and get_provider return a const std::string &.  Modify
>   code accordingly.
> 
> - Move can_enable method from 'class probe' to 'class
>   static_probe_ops'.
> 
> - Use const references when iterating on vector of objects.
> 
> This patch converts the generic probe interface (gdb/probe.[ch]) to
> C++, and also performs some cleanups that were on my TODO list for a
> while.
> 
> The main changes were the conversion of 'struct probe' to 'class
> probe', and 'struct probe_ops' to 'class static_probe_ops'.  The
> former now contains all the "dynamic", generic methods that act on a
> probe + the generic data related to it; the latter encapsulates a
> bunch of "static" methods that relate to the probe type, but not to a
> specific probe itself.
> 
> I've had to do a few renamings (e.g., on 'struct bound_probe' the
> field is called 'probe *prob' now, instead of 'struct probe *probe')
> because GCC was complaining about naming the field using the same name
> as the class.  Nothing major, though.  Generally speaking, the logic
> behind and the design behind the code are the same.
> 
> Even though I'm sending a series of patches, they need to be tested
> and committed as a single unit, because of inter-dependencies.  But it
> should be easier to review in separate logical units.
> 
> I've regtested this patch on BuildBot, no regressions found.

Hi Sergio,

I've looked at it (the series) quickly, and it looks good to me.  Personally,
for comparing std::string with char*, I would use the form

  str != char_ptr

rather than

  str.compare (char_ptr) != 0

since I find it more readable, but I don't really mind.

One whitespace comment below.

> @@ -334,56 +358,33 @@ compare_probes (const bound_probe &a, const bound_probe &b)
>  
>  static void
>  gen_ui_out_table_header_info (const std::vector<bound_probe> &probes,
> -			      const struct probe_ops *p)
> +			      const static_probe_ops *spops)
>  {
>    /* `headings' refers to the names of the columns when printing `info
>       probes'.  */
> -  VEC (info_probe_column_s) *headings = NULL;
> -  struct cleanup *c;
> -  info_probe_column_s *column;
> -  size_t headings_size;
> -  int ix;
> +  gdb_assert (spops != NULL);
>  
> -  gdb_assert (p != NULL);
> +  std::vector<struct info_probe_column> headings
> +    = spops->gen_info_probes_table_header ();
>  
> -  if (p->gen_info_probes_table_header == NULL
> -      && p->gen_info_probes_table_values == NULL)
> -    return;
> -
> -  gdb_assert (p->gen_info_probes_table_header != NULL
> -	      && p->gen_info_probes_table_values != NULL);
> -
> -  c = make_cleanup (VEC_cleanup (info_probe_column_s), &headings);
> -  p->gen_info_probes_table_header (&headings);
> -
> -  headings_size = VEC_length (info_probe_column_s, headings);
> -
> -  for (ix = 0;
> -       VEC_iterate (info_probe_column_s, headings, ix, column);
> -       ++ix)
> +  for (const struct info_probe_column &column : headings)
>      {
> -      size_t size_max = strlen (column->print_name);
> +      size_t size_max = strlen (column.print_name);
>  
>        for (const bound_probe &probe : probes)
>  	{
>  	  /* `probe_fields' refers to the values of each new field that this
>  	     probe will display.  */
> -	  VEC (const_char_ptr) *probe_fields = NULL;
> -	  struct cleanup *c2;
> -	  const char *val;
> -	  int kx;
>  
> -	  if (probe.probe->pops != p)
> +	  if (probe.prob->get_static_ops () != spops)
>  	    continue;
>  
> -	  c2 = make_cleanup (VEC_cleanup (const_char_ptr), &probe_fields);
> -	  p->gen_info_probes_table_values (probe.probe, &probe_fields);
> +  	  std::vector<const char *> probe_fields

There are spaces before the tab in this line.

Thanks,

Simon