[RFA] Convert observers to C++

[Pedro Alves]

On 02/10/2018 03:38 AM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> I'm running this through the buildbot again and when it is working I
> Tom> will re-submit it.
> 
> Here it is.

Thanks again.  I have a few comments / suggestions.  See below, and
the attached patches.

Offhand, let me say that after playing with this today, I found
that this being plain C++ instead of shell-based really makes it
much easier to play around with the implementation.  I like that,
a lot.

> +template<typename... T>
> +class observable
> +{
> +public:
> +
> +  typedef std::function<void(T...)> func_type;
> +
> +  /* The return type of attach, which can be passed to detach to
> +     remove a listener.  */
> +  typedef size_t token_type;
> +
> +  /* A "null" value of token_type that can be used as a
> +     sentinel.  */
> +  static constexpr const token_type null_token = size_t (-1);
> +
> +  explicit observable (const char *name)
> +    : m_count (0),
> +      m_name (name)
> +  {
> +  }
> +
> +  DISABLE_COPY_AND_ASSIGN (observable);
> +
> +  /* Attach F as an observable to this observable.  A token is
> +     returned which can be used to later remove F.  */
> +  token_type attach (const func_type &f)
> +  {
> +    m_observers.emplace_back (m_count, f);
> +    return m_count++;
> +  }


The counter is a size_t, so in principle at least for 64-bit
architectures, it shouldn't ever be a problem in practice (as in
risk of overflow), but my pedantic self still wants to try to see if
we can avoid forever-increasing counters by design.

How about we replace the counter by key that is managed by
the caller?  Very much similar to the registry mechanism.
In turn that allows registering multiple observers with
the same key, meaning at places where we need to
attach/detach a number of observers, we don't need to
track a token per each observable.  See patch #1 attached.

> +
> +  /* Remove an observer from this observable.  F is the token that was
> +     previously returned by "attach".  */
> +  void detach (const token_type &f)
> +  {
> +    auto iter = std::remove_if (m_observers.begin (),
> +				m_observers.end (),
> +				[=] (const std::pair<size_t, func_type> &e)
> +				{
> +				  return e.first == f;
> +				});
> +
> +    m_observers.erase (iter, m_observers.end ());
> +  }
> +
> +  /* Notify all observables that are attached to this observer.  */

I think observable and observer are swapped in this comment.  (see patch)

> +  void notify (T... args) const
> +  {
> +    if (observer_debug)
> +      fprintf_unfiltered (gdb_stdlog, "observer %s notify() called\n",
> +			  m_name);
> +    for (auto &&e : m_observers)
> +      e.second (args...);
> +  }
> +
> +private:
> +
> +  size_t m_count;
> +  std::vector<std::pair<size_t, func_type>> m_observers;
> +  const char *m_name;
> +};
> +
> +} /* namespace observers */
> +
> +} /* namespace gdb */



> --- a/gdb/observer.c
> +++ b/gdb/observer.c
> @@ -17,199 +17,199 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -/* An observer is an entity who is interested in being notified when GDB
> -   reaches certain states, or certain events occur in GDB.  The entity being
> -   observed is called the Subject.  To receive notifications, the observer
> -   attaches a callback to the subject.  One subject can have several
> -   observers.
> -
> -   This file implements an internal generic low-level event notification
> -   mechanism based on the Observer paradigm described in the book "Design
> -   Patterns".  This generic event notification mechansim is then re-used
> -   to implement the exported high-level notification management routines
> -   for all possible notifications.
> -
> -   The current implementation of the generic observer provides support
> -   for contextual data.  This contextual data is given to the subject
> -   when attaching the callback.  In return, the subject will provide
> -   this contextual data back to the observer as a parameter of the
> -   callback.
> -
> -   FIXME: The current support for the contextual data is only partial,
> -   as it lacks a mechanism that would deallocate this data when the
> -   callback is detached.  This is not a problem so far, as this contextual
> -   data is only used internally to hold a function pointer.  Later on,
> -   if a certain observer needs to provide support for user-level
> -   contextual data, then the generic notification mechanism will need
> -   need to be enhanced to allow the observer to provide a routine to
> -   deallocate the data when attaching the callback.
> -
> -   This file is currently maintained by hand, but the long term plan
> -   if the number of different notifications starts growing is to create
> -   a new script (observer.sh) that would generate this file, and the
> -   associated documentation.  */
> -
>  #include "defs.h"
>  #include "observer.h"
>  #include "command.h"
>  #include "gdbcmd.h"
> +#include "selftest.h"
>  
> -static unsigned int observer_debug;
> -static void
> -show_observer_debug (struct ui_file *file, int from_tty,
> -		     struct cmd_list_element *c, const char *value)
> +namespace gdb
>  {
> -  fprintf_filtered (file, _("Observer debugging is %s.\n"), value);
> -}
> -
> -/* The internal generic observer.  */
>  
> -typedef void (generic_observer_notification_ftype) (const void *data,
> -						    const void *args);
> -
> -struct observer
> +namespace observers
>  {
> -  generic_observer_notification_ftype *notify;
> -  /* No memory management needed for the following field for now.  */
> -  void *data;
> -};
> -
> -/* A list of observers, maintained by the subject.  A subject is
> -   actually represented by its list of observers.  */
>  
> -struct observer_list
> +unsigned int observer_debug;
> +
> +#define DEFINE_OBSERVER(name) decltype (name) name (# name)

Hmm, pedantically, I guess this should be
called DEFINE_OBSERVABLE instead of DEFINE_OBSERVER?
(see patch #3).

> +
> +DEFINE_OBSERVER (normal_stop);
> +DEFINE_OBSERVER (signal_received);
> +DEFINE_OBSERVER (end_stepping_range);


...

> +
> +#ifdef GDB_SELF_TEST
> +
> +namespace selftests
>  {
> -  struct observer_list *next;
> -  struct observer *observer;
> -};
>  
> -/* Allocate a struct observer_list, intended to be used as a node
> -   in the list of observers maintained by a subject.  */
> +/* This observer is used for internal testing.  */
> +observable<int> test_notification ("test_notification");

We have the gdb/unittests/ directory nowadays, so I think
we could/should move the tests there.  See patch #2.

And with patches #2 and #3, I wonder whether gdb/observer.{h,c}
is the right name for the files, given that they contain
definitions of observables, not observers?  Maybe
they should be called gdb/observables.{h,c} instead (plural).
Dunno, it's not a big deal.  Fine with me as is, though I
wonder whether that's because I'm used to the nomenclature;
i.e., whether a newcomer would feel the same.

WDYT?

Thanks,
Pedro Alves
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-address-indentify-tokens-instead-of-counters.-Fi.patch
Type: text/x-patch
Size: 12263 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20180212/7d9df38a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Move-unit-tests-to-gdb-unittests.patch
Type: text/x-patch
Size: 8777 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20180212/7d9df38a/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-DEFINE_OBSERVER-DEFINE_OBSERVABLE.patch
Type: text/x-patch
Size: 3864 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20180212/7d9df38a/attachment-0002.bin>