From patchwork Mon Feb 12 18:43:08 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 25917 Received: (qmail 77720 invoked by alias); 12 Feb 2018 18:43:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 77706 invoked by uid 89); 12 Feb 2018 18:43:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_LOW, T_FILL_THIS_FORM_SHORT, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=ATTACH, 2079 X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Feb 2018 18:43:11 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 785EC4023141; Mon, 12 Feb 2018 18:43:09 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id AED0A2166BAE; Mon, 12 Feb 2018 18:43:08 +0000 (UTC) Subject: Re: [RFA] Convert observers to C++ To: Tom Tromey References: <20171105013039.9135-1-tom@tromey.com> <20f70b2b-bf93-a6b6-f2a4-82e14c807e05@redhat.com> <87zi4haeak.fsf@tromey.com> <87vaf5a4zq.fsf@tromey.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <18b73858-b80f-4655-d33c-2be0dce650eb@redhat.com> Date: Mon, 12 Feb 2018 18:43:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <87vaf5a4zq.fsf@tromey.com> On 02/10/2018 03:38 AM, Tom Tromey wrote: >>>>>> "Tom" == Tom Tromey 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 > +class observable > +{ > +public: > + > + typedef std::function 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 &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> 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 . */ > > -/* 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 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 From 5618a1c3d564387dff4799538e16849d2b1df816 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 12 Feb 2018 17:55:14 +0000 Subject: [PATCH 3/3] DEFINE_OBSERVER -> DEFINE_OBSERVABLE --- gdb/observer.c | 86 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/gdb/observer.c b/gdb/observer.c index 836b083125d..d85ecd2cce7 100644 --- a/gdb/observer.c +++ b/gdb/observer.c @@ -30,50 +30,50 @@ namespace observers unsigned int observer_debug; -#define DEFINE_OBSERVER(name) decltype (name) name (# name) +#define DEFINE_OBSERVABLE(name) decltype (name) name (# name) -DEFINE_OBSERVER (normal_stop); -DEFINE_OBSERVER (signal_received); -DEFINE_OBSERVER (end_stepping_range); -DEFINE_OBSERVER (signal_exited); -DEFINE_OBSERVER (exited); -DEFINE_OBSERVER (no_history); -DEFINE_OBSERVER (sync_execution_done); -DEFINE_OBSERVER (command_error); -DEFINE_OBSERVER (target_changed); -DEFINE_OBSERVER (executable_changed); -DEFINE_OBSERVER (inferior_created); -DEFINE_OBSERVER (record_changed); -DEFINE_OBSERVER (solib_loaded); -DEFINE_OBSERVER (solib_unloaded); -DEFINE_OBSERVER (new_objfile); -DEFINE_OBSERVER (free_objfile); -DEFINE_OBSERVER (new_thread); -DEFINE_OBSERVER (thread_exit); -DEFINE_OBSERVER (thread_stop_requested); -DEFINE_OBSERVER (target_resumed); -DEFINE_OBSERVER (about_to_proceed); -DEFINE_OBSERVER (breakpoint_created); -DEFINE_OBSERVER (breakpoint_deleted); -DEFINE_OBSERVER (breakpoint_modified); -DEFINE_OBSERVER (traceframe_changed); -DEFINE_OBSERVER (architecture_changed); -DEFINE_OBSERVER (thread_ptid_changed); -DEFINE_OBSERVER (inferior_added); -DEFINE_OBSERVER (inferior_appeared); -DEFINE_OBSERVER (inferior_exit); -DEFINE_OBSERVER (inferior_removed); -DEFINE_OBSERVER (memory_changed); -DEFINE_OBSERVER (before_prompt); -DEFINE_OBSERVER (gdb_datadir_changed); -DEFINE_OBSERVER (command_param_changed); -DEFINE_OBSERVER (tsv_created); -DEFINE_OBSERVER (tsv_deleted); -DEFINE_OBSERVER (tsv_modified); -DEFINE_OBSERVER (inferior_call_pre); -DEFINE_OBSERVER (inferior_call_post); -DEFINE_OBSERVER (register_changed); -DEFINE_OBSERVER (user_selected_context_changed); +DEFINE_OBSERVABLE (normal_stop); +DEFINE_OBSERVABLE (signal_received); +DEFINE_OBSERVABLE (end_stepping_range); +DEFINE_OBSERVABLE (signal_exited); +DEFINE_OBSERVABLE (exited); +DEFINE_OBSERVABLE (no_history); +DEFINE_OBSERVABLE (sync_execution_done); +DEFINE_OBSERVABLE (command_error); +DEFINE_OBSERVABLE (target_changed); +DEFINE_OBSERVABLE (executable_changed); +DEFINE_OBSERVABLE (inferior_created); +DEFINE_OBSERVABLE (record_changed); +DEFINE_OBSERVABLE (solib_loaded); +DEFINE_OBSERVABLE (solib_unloaded); +DEFINE_OBSERVABLE (new_objfile); +DEFINE_OBSERVABLE (free_objfile); +DEFINE_OBSERVABLE (new_thread); +DEFINE_OBSERVABLE (thread_exit); +DEFINE_OBSERVABLE (thread_stop_requested); +DEFINE_OBSERVABLE (target_resumed); +DEFINE_OBSERVABLE (about_to_proceed); +DEFINE_OBSERVABLE (breakpoint_created); +DEFINE_OBSERVABLE (breakpoint_deleted); +DEFINE_OBSERVABLE (breakpoint_modified); +DEFINE_OBSERVABLE (traceframe_changed); +DEFINE_OBSERVABLE (architecture_changed); +DEFINE_OBSERVABLE (thread_ptid_changed); +DEFINE_OBSERVABLE (inferior_added); +DEFINE_OBSERVABLE (inferior_appeared); +DEFINE_OBSERVABLE (inferior_exit); +DEFINE_OBSERVABLE (inferior_removed); +DEFINE_OBSERVABLE (memory_changed); +DEFINE_OBSERVABLE (before_prompt); +DEFINE_OBSERVABLE (gdb_datadir_changed); +DEFINE_OBSERVABLE (command_param_changed); +DEFINE_OBSERVABLE (tsv_created); +DEFINE_OBSERVABLE (tsv_deleted); +DEFINE_OBSERVABLE (tsv_modified); +DEFINE_OBSERVABLE (inferior_call_pre); +DEFINE_OBSERVABLE (inferior_call_post); +DEFINE_OBSERVABLE (register_changed); +DEFINE_OBSERVABLE (user_selected_context_changed); } /* namespace observers */ } /* namespace gdb */ -- 2.14.3