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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, November 22 2017, Simon Marchi wrote:

> 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,

Hey Simon,

Thanks for the review.

> 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.

You're right, it's better.  I'll replace that in my code.

> 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 for catching that.  Fixed.

Is it OK to push after these changes, or do you want me to submit a v2?

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/