From 7eda212753057c07f429dfdfb0cf3a18312de054 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Tue, 28 Dec 2021 21:46:57 +0100 Subject: Refactor EventFactory and move it out of _impl:: Strictly speaking, EventFactory can be further instantiated if any client application figures they need a whole new base class for events and respectively a separate EventFactory specialisation for it. Where this whole commit started though was a linkage error because I did not plan to expose Quotient-specific logging categories for linkage (effectively, usage) from the client code - meanwhile the inline code of EventFactory uses qDebug(EVENTS), meaning I had to either add QUOTIENT_API to EVENTS or hide those invocations. This in turn led to trimming the EventFactory constructor back to trivial implementation and dropping the guard variable that was supposed to trace duplicate EventFactory objects for the same BaseEventT - with the reasoning that such situation is not really dangerous (unlike EventTypeRegistry double-initialisation fiasco, see #413), and at the same time it can be easily detected in the logs by duplicated factory method registration messages. And while I was at it, I replaced the meaningless bool in the return type of EventFactory<>::addMethod with the slightly more (but still barely) useful reference to the inserted factory method. One can (in theory) use it now if they need to turn some event JSON into an object of some specific event type or nullptr if the event type in the JSON payload doesn't match - but at the same rate (for now at least) one can call makeIfMatches() directly. With this commit, both Quotest and Quaternion build and link using either Clang or GCC even under -fvisibility=hidden. However, running quotest now reproduces #413, which is a matter of event typeId infrastructure refactoring, coming in further commits. --- lib/events/event.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'lib/events/event.cpp') diff --git a/lib/events/event.cpp b/lib/events/event.cpp index 96be717c..715e7da2 100644 --- a/lib/events/event.cpp +++ b/lib/events/event.cpp @@ -27,6 +27,14 @@ QString EventTypeRegistry::getMatrixType(event_type_t typeId) : QString(); } +void _impl::EventFactoryBase::logAddingMethod(event_mtype_t matrixType, + size_t newSize) +{ + qDebug(EVENTS) << "Adding factory method for" << matrixType << "events;" + << newSize << "methods will be in the" << name + << "chain"; +} + Event::Event(Type type, const QJsonObject& json) : _type(type), _json(json) { if (!json.contains(ContentKeyL) -- cgit v1.2.3 From 27bb7ba696ae803c6a6903f85fe14074b23b7bcc Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Tue, 28 Dec 2021 21:34:03 +0100 Subject: Use QLatin1String for event typeId's Before all, this fixes the problem with double-initialising of type ids; it could have been fixed with a smaller change but EventTypeRegistry is fairly superfluous now when inline variables are a thing and it's possible to have an extensible registry system using literally pointers to the memory that are guaranteed to be unique. That being said, event_type_t is still QLatin1String and not a bare const char* (or void*), mostly to stay on the safe side when it comes to type identities: unlike const char*, QLatin1String's are deep-compared, meaning that matching for switchOnType (former visit) occurs a bit slower now. This may change in the future; but this is the first step in getting rid of EventTypeRegistry. This change means that initializeTypeId is no more needed; also, two static member functions, typeId() and matrixTypeId(), are being replaced with a single inline static member variable, TypeId. This commit doesn't apply that transition across the event types, meaning that you'll get a pile of warnings when compiling the library. These warnings will be tackled in further commits within this branch. --- lib/events/event.cpp | 22 +++-------------- lib/events/event.h | 68 ++++++++++++++++------------------------------------ 2 files changed, 23 insertions(+), 67 deletions(-) (limited to 'lib/events/event.cpp') diff --git a/lib/events/event.cpp b/lib/events/event.cpp index 715e7da2..4c304a3c 100644 --- a/lib/events/event.cpp +++ b/lib/events/event.cpp @@ -9,28 +9,12 @@ using namespace Quotient; -event_type_t EventTypeRegistry::initializeTypeId(event_mtype_t matrixTypeId) -{ - const auto id = get().eventTypes.size(); - get().eventTypes.push_back(matrixTypeId); - if (strncmp(matrixTypeId, "", 1) == 0) - qDebug(EVENTS) << "Initialized unknown event type with id" << id; - else - qDebug(EVENTS) << "Initialized event type" << matrixTypeId << "with id" - << id; - return id; -} - -QString EventTypeRegistry::getMatrixType(event_type_t typeId) -{ - return typeId < get().eventTypes.size() ? get().eventTypes[typeId] - : QString(); -} +QString EventTypeRegistry::getMatrixType(event_type_t typeId) { return typeId; } -void _impl::EventFactoryBase::logAddingMethod(event_mtype_t matrixType, +void _impl::EventFactoryBase::logAddingMethod(event_type_t TypeId, size_t newSize) { - qDebug(EVENTS) << "Adding factory method for" << matrixType << "events;" + qDebug(EVENTS) << "Adding factory method for" << TypeId << "events;" << newSize << "methods will be in the" << name << "chain"; } diff --git a/lib/events/event.h b/lib/events/event.h index 0aef49f7..47f07c1d 100644 --- a/lib/events/event.h +++ b/lib/events/event.h @@ -55,60 +55,32 @@ inline QJsonObject basicEventJson(const QString& matrixType, return { { TypeKey, matrixType }, { ContentKey, content } }; } -// === Event types and event types registry === +// === Event types === -using event_type_t = size_t; +using event_type_t = QLatin1String; using event_mtype_t = const char*; class QUOTIENT_API EventTypeRegistry { public: ~EventTypeRegistry() = default; - static event_type_t initializeTypeId(event_mtype_t matrixTypeId); - - template - static event_type_t initializeTypeId() - { - return initializeTypeId(EventT::matrixTypeId()); - } - + [[deprecated("event_type_t is a string now, use it directly instead")]] static QString getMatrixType(event_type_t typeId); private: EventTypeRegistry() = default; Q_DISABLE_COPY_MOVE(EventTypeRegistry) - - static EventTypeRegistry& get() - { - static EventTypeRegistry etr; - return etr; - } - - std::vector eventTypes; -}; - -template <> -inline event_type_t EventTypeRegistry::initializeTypeId() -{ - return initializeTypeId(""); -} - -template -struct EventTypeTraits { - static event_type_t id() - { - static const auto id = EventTypeRegistry::initializeTypeId(); - return id; - } }; template inline event_type_t typeId() { - return EventTypeTraits>::id(); + return std::decay_t::TypeId; } -inline event_type_t unknownEventTypeId() { return typeId(); } +constexpr inline event_type_t UnknownEventTypeId = "?"_ls; +[[deprecated("Use UnknownEventTypeId")]] +constexpr inline event_type_t unknownEventTypeId() { return UnknownEventTypeId; } // === Event creation facilities === @@ -128,7 +100,7 @@ namespace _impl { explicit EventFactoryBase(const char* name) : name(name) {} - void logAddingMethod(event_mtype_t mtypeId, size_t newSize); + void logAddingMethod(event_type_t TypeId, size_t newSize); private: const char* const name; @@ -155,9 +127,9 @@ private: static event_ptr_tt makeIfMatches(const QJsonObject& json, const QString& matrixType) { - return QLatin1String(EventT::matrixTypeId()) == matrixType - ? makeEvent(json) - : nullptr; + // If your matrix event type is not all ASCII, it's your problem + // (see https://github.com/matrix-org/matrix-doc/pull/2758) + return EventT::TypeId == matrixType ? makeEvent(json) : nullptr; } public: @@ -175,7 +147,7 @@ public: template const auto& addMethod() { - logAddingMethod(EventT::matrixTypeId(), methods.size() + 1); + logAddingMethod(EventT::TypeId, methods.size() + 1); return methods.emplace_back(&makeIfMatches); } @@ -184,7 +156,7 @@ public: for (const auto& f : methods) if (auto e = f(json, matrixType)) return e; - return makeEvent(unknownEventTypeId(), json); + return makeEvent(UnknownEventTypeId, json); } }; @@ -285,12 +257,12 @@ using Events = EventsArray; // This macro should be used in a public section of an event class to // provide matrixTypeId() and typeId(). -#define DEFINE_EVENT_TYPEID(_Id, _Type) \ - static QUOTIENT_API constexpr event_mtype_t matrixTypeId() \ - { \ - return _Id; \ - } \ - static QUOTIENT_API auto typeId() { return Quotient::typeId<_Type>(); } \ +#define DEFINE_EVENT_TYPEID(Id_, Type_) \ + static inline constexpr event_type_t TypeId = Id_##_ls; \ + [[deprecated("Use _Type::TypeId directly instead")]] \ + static constexpr event_mtype_t matrixTypeId() { return Id_; } \ + [[deprecated("Use _Type::TypeId directly instead")]] \ + static event_type_t typeId() { return TypeId; } \ // End of macro // This macro should be put after an event class definition (in .h or .cpp) @@ -311,7 +283,7 @@ inline bool is(const Event& e) inline bool isUnknown(const Event& e) { - return e.type() == unknownEventTypeId(); + return e.type() == UnknownEventTypeId; } template -- cgit v1.2.3