Simplify the code around EventFactory<>

The former code assumed that EventFactory<> is just a class-level shell
for a bunch of functions and a static data member that only exists to
allow specialisations to occur for the whole group together. On top of
that, setupFactory() and registerEventType() strived to protect this
group from double registration coming from static variables in an
anonymous namespace produced by REGISTER_EVENT_TYPE.

The whole thing is now de-static-ed: resolving the factory now relies
on class-static Event/RoomEvent/StateEventBase::factory variables
instead of factory_t type aliases; and REGISTER_EVENT_TYPE produces
non-static inline variables instead, obviating the need of
registerEventType/setupFactory kludge.

6 files changed, 125 insertions, 126 deletions

diff --git a/lib/events/event.h b/lib/events/event.h
index 4d4bb16b..e786fb30 100644
--- a/lib/events/event.h
+++ b/lib/events/event.h
@@ -110,94 +110,90 @@ inline event_type_t typeId()
 
 inline event_type_t unknownEventTypeId() { return typeId<void>(); }
 
-// === EventFactory ===
+// === Event creation facilities ===
 
-/** Create an event of arbitrary type from its arguments */
+//! Create an event of arbitrary type from its arguments
 template <typename EventT, typename... ArgTs>
 inline event_ptr_tt<EventT> makeEvent(ArgTs&&... args)
 {
     return std::make_unique<EventT>(std::forward<ArgTs>(args)...);
 }
 
-template <typename BaseEventT>
-class EventFactory {
-public:
-    template <typename FnT>
-    static auto addMethod(FnT&& method)
-    {
-        factories().emplace_back(std::forward<FnT>(method));
-        return 0;
-    }
-
-    /** Chain two type factories
-     * Adds the factory class of EventT2 (EventT2::factory_t) to
-     * the list in factory class of EventT1 (EventT1::factory_t) so
-     * that when EventT1::factory_t::make() is invoked, types of
-     * EventT2 factory are looked through as well. This is used
-     * to include RoomEvent types into the more general Event factory,
-     * and state event types into the RoomEvent factory.
-     */
-    template <typename EventT>
-    static auto chainFactory()
-    {
-        return addMethod(&EventT::factory_t::make);
-    }
-
-    static event_ptr_tt<BaseEventT> make(const QJsonObject& json,
-                                         const QString& matrixType)
-    {
-        for (const auto& f : factories())
-            if (auto e = f(json, matrixType))
-                return e;
-        return nullptr;
-    }
-
-private:
-    static auto& factories()
+namespace _impl {
+    template <class EventT, class BaseEventT>
+    event_ptr_tt<BaseEventT> makeIfMatches(const QJsonObject& json,
+                                           const QString& matrixType)
     {
-        using inner_factory_tt = std::function<event_ptr_tt<BaseEventT>(
-            const QJsonObject&, const QString&)>;
-        static std::vector<inner_factory_tt> _factories {};
-        return _factories;
+        return QLatin1String(EventT::matrixTypeId()) == matrixType
+                   ? makeEvent<EventT>(json)
+                   : nullptr;
     }
-};
-
-/** Add a type to its default factory
- * Adds a standard factory method (via makeEvent<>) for a given
- * type to EventT::factory_t factory class so that it can be
- * created dynamically from loadEvent<>().
- *
- * \tparam EventT the type to enable dynamic creation of
- * \return the registered type id
- * \sa loadEvent, Event::type
- */
-template <typename EventT>
-inline auto setupFactory()
-{
-    qDebug(EVENTS) << "Adding factory method for" << EventT::matrixTypeId();
-    return EventT::factory_t::addMethod([](const QJsonObject& json,
-                                           const QString& jsonMatrixType) {
-        return EventT::matrixTypeId() == jsonMatrixType ? makeEvent<EventT>(json)
-                                                        : nullptr;
-    });
-}
 
-template <typename EventT>
-inline auto registerEventType()
-{
-    // Initialise exactly once, even if this function is called twice for
-    // the same type (for whatever reason - you never know the ways of
-    // static initialisation is done).
-    static const auto _ = setupFactory<EventT>();
-    return _; // Only to facilitate usage in static initialisation
-}
+    //! \brief A family of event factories to create events from CS API responses
+    //!
+    //! Each of these factories, as instantiated by event base types (Event,
+    //! RoomEvent etc.) is capable of producing an event object derived from
+    //! \p BaseEventT, using the JSON payload and the event type passed to its
+    //! make() method. Don't use these directly to make events; use loadEvent()
+    //! overloads as the frontend for these. Never instantiate new factories
+    //! outside of base event classes.
+    //! \sa loadEvent, setupFactory, Event::factory, RoomEvent::factory,
+    //!     StateEventBase::factory
+    template <typename BaseEventT>
+    class EventFactory
+        : private std::vector<event_ptr_tt<BaseEventT> (*)(const QJsonObject&,
+                                                           const QString&)> {
+        // Actual makeIfMatches specialisations will differ in the first
+        // template parameter but that doesn't affect the function type
+    public:
+        explicit EventFactory(const char* name = "")
+            : name(name)
+        {
+            static auto yetToBeConstructed = true;
+            Q_ASSERT(yetToBeConstructed);
+            if (!yetToBeConstructed) // For Release builds that pass Q_ASSERT
+                qCritical(EVENTS)
+                    << "Another EventFactory for the same base type is being "
+                       "created - event creation logic will be splintered";
+            yetToBeConstructed = false;
+        }
+        EventFactory(const EventFactory&) = delete;
+
+        //! \brief Add a method to create events of a given type
+        //!
+        //! Adds a standard factory method (makeIfMatches) for \p EventT so that
+        //! event objects of this type can be created dynamically by loadEvent.
+        //! The caller is responsible for ensuring this method is called only
+        //! once per type.
+        //! \sa makeIfMatches, loadEvent, Quotient::loadEvent
+        template <class EventT>
+        bool addMethod()
+        {
+            this->emplace_back(&makeIfMatches<EventT, BaseEventT>);
+            qDebug(EVENTS) << "Added factory method for"
+                           << EventT::matrixTypeId() << "events;" << this->size()
+                           << "methods in the" << name << "chain by now";
+            return true;
+        }
+
+        auto loadEvent(const QJsonObject& json, const QString& matrixType)
+        {
+            for (const auto& f : *this)
+                if (auto e = f(json, matrixType))
+                    return e;
+            return makeEvent<BaseEventT>(unknownEventTypeId(), json);
+        }
+
+        const char* const name;
+    };
+} // namespace _impl
 
 // === Event ===
 
 class Event {
 public:
     using Type = event_type_t;
-    using factory_t = EventFactory<Event>;
+    static inline _impl::EventFactory<Event> factory { "Event" };
 
     explicit Event(Type type, const QJsonObject& json);
     explicit Event(Type type, event_mtype_t matrixType,
@@ -272,7 +268,7 @@ template <typename EventT>
 using EventsArray = std::vector<event_ptr_tt<EventT>>;
 using Events = EventsArray<Event>;
 
-// === Macros used with event class definitions ===
+// === Facilities for event class definitions ===
 
 // This macro should be used in a public section of an event class to
 // provide matrixTypeId() and typeId().
@@ -284,13 +280,27 @@ using Events = EventsArray<Event>;
 // This macro should be put after an event class definition (in .h or .cpp)
 // to enable its deserialisation from a /sync and other
 // polymorphic event arrays
-#define REGISTER_EVENT_TYPE(_Type)                                \
-    namespace {                                                   \
-        [[maybe_unused]] static const auto _factoryAdded##_Type = \
-            registerEventType<_Type>();                           \
-    }                                                             \
+#define REGISTER_EVENT_TYPE(_Type)                            \
+    [[maybe_unused]] inline const auto _factoryAdded##_Type = \
+        _Type::factory.addMethod<_Type>();                    \
     // End of macro
 
+// === Event loading ===
+// (see also event_loader.h)
+
+//! \brief Point of customisation to dynamically load events
+//!
+//! The default specialisation of this calls BaseEventT::factory and if that
+//! fails (i.e. returns nullptr) creates an unknown event of BaseEventT.
+//! Other specialisations may reuse other factories, add validations common to
+//! BaseEventT, and so on
+template <class BaseEventT>
+event_ptr_tt<BaseEventT> doLoadEvent(const QJsonObject& json,
+                                     const QString& matrixType)
+{
+    return BaseEventT::factory.loadEvent(json, matrixType);
+}
+
 // === is<>(), eventCast<>() and switchOnType<>() ===
 
 template <class EventT>
diff --git a/lib/events/eventloader.h b/lib/events/eventloader.h
index 978668f2..fe624d70 100644
--- a/lib/events/eventloader.h
+++ b/lib/events/eventloader.h
@@ -6,16 +6,6 @@
 #include "stateevent.h"
 
 namespace Quotient {
-namespace _impl {
-    template <typename BaseEventT>
-    static inline auto loadEvent(const QJsonObject& json,
-                                 const QString& matrixType)
-    {
-        if (auto e = EventFactory<BaseEventT>::make(json, matrixType))
-            return e;
-        return makeEvent<BaseEventT>(unknownEventTypeId(), json);
-    }
-} // namespace _impl
 
 /*! Create an event with proper type from a JSON object
  *
@@ -26,7 +16,7 @@ namespace _impl {
 template <typename BaseEventT>
 inline event_ptr_tt<BaseEventT> loadEvent(const QJsonObject& fullJson)
 {
-    return _impl::loadEvent<BaseEventT>(fullJson, fullJson[TypeKeyL].toString());
+    return doLoadEvent<BaseEventT>(fullJson, fullJson[TypeKeyL].toString());
 }
 
 /*! Create an event from a type string and content JSON
@@ -39,8 +29,8 @@ template <typename BaseEventT>
 inline event_ptr_tt<BaseEventT> loadEvent(const QString& matrixType,
                                           const QJsonObject& content)
 {
-    return _impl::loadEvent<BaseEventT>(basicEventJson(matrixType, content),
-                                        matrixType);
+    return doLoadEvent<BaseEventT>(basicEventJson(matrixType, content),
+                                   matrixType);
 }
 
 /*! Create a state event from a type string, content JSON and state key
@@ -53,7 +43,7 @@ inline StateEventPtr loadStateEvent(const QString& matrixType,
                                     const QJsonObject& content,
                                     const QString& stateKey = {})
 {
-    return _impl::loadEvent<StateEventBase>(
+    return doLoadEvent<StateEventBase>(
         basicStateEventJson(matrixType, content, stateKey), matrixType);
 }
 
diff --git a/lib/events/roomevent.h b/lib/events/roomevent.h
index 7f13f6f2..8be58481 100644
--- a/lib/events/roomevent.h
+++ b/lib/events/roomevent.h
@@ -13,7 +13,7 @@ class RedactionEvent;
 /** This class corresponds to m.room.* events */
 class RoomEvent : public Event {
 public:
-    using factory_t = EventFactory<RoomEvent>;
+    static inline _impl::EventFactory<RoomEvent> factory { "RoomEvent" };
 
     // RedactionEvent is an incomplete type here so we cannot inline
     // constructors and destructors and we cannot use 'using'.
diff --git a/lib/events/roommemberevent.h b/lib/events/roommemberevent.h
index f3047159..0fb464d4 100644
--- a/lib/events/roommemberevent.h
+++ b/lib/events/roommemberevent.h
@@ -49,16 +49,15 @@ public:
                      std::forward<ArgTs>(contentArgs)...)
     {}
 
-    /// A special constructor to create unknown RoomMemberEvents
-    /**
-     * This is needed in order to use RoomMemberEvent as a "base event
-     * class" in cases like GetMembersByRoomJob when RoomMemberEvents
-     * (rather than RoomEvents or StateEvents) are resolved from JSON.
-     * For such cases loadEvent<> requires an underlying class to be
-     * constructible with unknownTypeId() instead of its genuine id.
-     * Don't use it directly.
-     * \sa GetMembersByRoomJob, loadEvent, unknownTypeId
-     */
+    //! \brief A special constructor to create unknown RoomMemberEvents
+    //!
+    //! This is needed in order to use RoomMemberEvent as a "base event class"
+    //! in cases like GetMembersByRoomJob when RoomMemberEvents (rather than
+    //! RoomEvents or StateEvents) are resolved from JSON. For such cases
+    //! loadEvent\<> requires an underlying class to have a specialisation of
+    //! EventFactory\<> and be constructible with unknownTypeId() instead of
+    //! its genuine id. Don't use directly.
+    //! \sa EventFactory, loadEvent, GetMembersByRoomJob
     RoomMemberEvent(Type type, const QJsonObject& fullJson)
         : StateEvent(type, fullJson)
     {}
@@ -89,14 +88,13 @@ public:
 };
 
 template <>
-class EventFactory<RoomMemberEvent> {
-public:
-    static event_ptr_tt<RoomMemberEvent> make(const QJsonObject& json,
-                                              const QString&)
-    {
+inline event_ptr_tt<RoomMemberEvent>
+doLoadEvent<RoomMemberEvent>(const QJsonObject& json, const QString& matrixType)
+{
+    if (matrixType == QLatin1String(RoomMemberEvent::matrixTypeId()))
         return makeEvent<RoomMemberEvent>(json);
-    }
-};
+    return makeEvent<RoomMemberEvent>(unknownEventTypeId(), json);
+}
 
 REGISTER_EVENT_TYPE(RoomMemberEvent)
 } // namespace Quotient
diff --git a/lib/events/stateevent.cpp b/lib/events/stateevent.cpp
index efe011a0..9535af56 100644
--- a/lib/events/stateevent.cpp
+++ b/lib/events/stateevent.cpp
@@ -5,21 +5,6 @@
 
 using namespace Quotient;
 
-// Aside from the normal factory to instantiate StateEventBase inheritors
-// StateEventBase itself can be instantiated if there's a state_key JSON key
-// but the event type is unknown.
-[[maybe_unused]] static auto stateEventTypeInitialised =
-    RoomEvent::factory_t::addMethod(
-        [](const QJsonObject& json, const QString& matrixType) -> StateEventPtr {
-            if (!json.contains(StateKeyKeyL))
-                return nullptr;
-
-            if (auto e = StateEventBase::factory_t::make(json, matrixType))
-                return e;
-
-            return makeEvent<StateEventBase>(unknownEventTypeId(), json);
-        });
-
 StateEventBase::StateEventBase(Event::Type type, event_mtype_t matrixType,
                                const QString& stateKey,
                                const QJsonObject& contentJson)
diff --git a/lib/events/stateevent.h b/lib/events/stateevent.h
index b0aa9907..919e8f86 100644
--- a/lib/events/stateevent.h
+++ b/lib/events/stateevent.h
@@ -19,7 +19,7 @@ inline QJsonObject basicStateEventJson(const QString& matrixTypeId,
 
 class StateEventBase : public RoomEvent {
 public:
-    using factory_t = EventFactory<StateEventBase>;
+    static inline _impl::EventFactory<StateEventBase> factory { "StateEvent" };
 
     StateEventBase(Type type, const QJsonObject& json) : RoomEvent(type, json)
     {}
@@ -37,6 +37,22 @@ public:
 using StateEventPtr = event_ptr_tt<StateEventBase>;
 using StateEvents = EventsArray<StateEventBase>;
 
+//! \brief Override RoomEvent factory with that from StateEventBase if JSON has
+//! stateKey
+//!
+//! This means in particular that an event with a type known to RoomEvent but
+//! having stateKey set (even to an empty value) will be treated as a state
+//! event and most likely end up as unknown (consider, e.g., m.room.message
+//! that has stateKey set).
+template <>
+inline RoomEventPtr doLoadEvent(const QJsonObject& json,
+                                const QString& matrixType)
+{
+    if (json.contains(StateKeyKeyL))
+        return StateEventBase::factory.loadEvent(json, matrixType);
+    return RoomEvent::factory.loadEvent(json, matrixType);
+}
+
 template <>
 inline bool is<StateEventBase>(const Event& e)
 {