Re: [RFA] Convert observers to C++

[Tom Tromey <tom at tromey dot com>]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Responding to some old-ish mail...

>> This converts observers from using a special source-generating script
>> to be plain C++.
[...]

>> -Note that the @code{normal_stop} notification is not emitted when
>> -the execution stops due to a breakpoint, and this breakpoint has
>> -a condition that is not met.  If the breakpoint has any associated
>> -commands list, the commands are executed after the notification
>> -is emitted.

Pedro> Is all the documentation in this file preserved?  I couldn't find
Pedro> where this paragraph was moved to, for example.

I've now moved this paragraph in particular into a comment.  The rest is
either preserved or not relevant (IMO).  I've added some comments to the
public methods of the class.

Pedro> And then, you could even do:
Pedro>  #define DEFINE_OBSERVER(OBSERVER_NAME) \
Pedro>    decltype (OBSERVER_NAME) OBSERVER_NAME (#OBSERVER_NAME)
Pedro>  DEFINE_OBSERVER (normal_stop);

I did this.

Pedro> It'd be nicer if the self tests were under "namespace selftests",
Pedro> IMHO.

Done.

Pedro> OOC, is there some guideline you were following for preferring
Pedro> "namespace gdb_observers" over, say, "namespace gdb::observers" ?
Pedro> Just curiosity, don't feel the need to change anything.

Nope.  I went ahead and moved it to gdb::observers, since it was easy.

Pedro> You're going to hate me, but it'd be nice IMO if you
Pedro> switched to use /**/ consistently throughout, per GCC/GDB
Pedro> convention.

I did this.  This was actually easy too.

>> +    m_observers.erase (m_observers.begin () + f);

Pedro> Hmm, this looks incorrect.  "attach" returns an index
Pedro> as token.  And then detach uses "std::vector::erase()",
Pedro> which removes the element at the index and then shifts
Pedro> the rest of the elements left to fill in the space
Pedro> left over by the removed element(s).  So that invalidates
Pedro> all token after the element erased.

Yes, thanks for catching this.

I figure that observers aren't very space-sensitive, so I just put a
counter into the observer and then put the value into the vector, like:

  std::vector<std::pair<size_t, func_type>> m_observers;

Pedro> This could be constexpr and then defined in-class, right?

Did it.

Pedro> (I wonder about splitting core/lib from uses of the core.
Pedro> I.e., defining the struct observer core mechanism in one
Pedro> header, and then define the actual observers in a separate header.
Pedro> I'm basically pondering about e.g., using observers under common/
Pedro> or in gdbserver/.  Like e.g., "common/observer.h" and then
Pedro> "gdb/observers.h" and "gdbserver/observers.h", etc.  But we can cross
Pedro> that bridge when we get to it, too.)

I did this.  Also I realized that calling the class "observer" is
somewhat wrong, as the callback is the observer -- so I made
common/observable.h and I renamed the class to "class observable".

I'm running this through the buildbot again and when it is working I
will re-submit it.

Tom