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

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

On Tuesday, November 14 2017, Simon Marchi wrote:

> Damn, I hit send too soon... here's review part 2.  Actually, there's
> wasn't much left.

Thanks.

> On 2017-11-14 09:52 PM, Simon Marchi wrote:
>> On 2017-11-13 12:58 PM, Sergio Durigan Junior wrote:
>>> 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.
>> 
>> Personally I'm fine with this.  Maybe there would be a better C++-ish
>> design, but I'm not very good at that.  If others have suggestions, I'd
>> like to hear them.
>
> Given your description, I think "class probe_type" would be a good name
> instead of static_probe_ops.

You know, "class probe_type" was one of the many names that crossed my
mind.  I didn't like it because it doesn't convey the meaning that I
want here, which is that this class is "generic".  I still think
"static_probe_ops" is a better fit.

>>> -extern const struct probe_ops probe_ops_any;
>>> +  /* Return true if the probe can be enabled; false otherwise.  */
>>> +  virtual bool can_enable () const
>>> +  {
>>> +    return false;
>>> +  }
>
> Should this be in the static_probe_ops?  Is the fact that a probe can be
> enabled/disabled a property of the probe, or the probe type?

You're right, it should be on static_probe_ops indeed.  I'll move it
there.

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/