Patch Detail
Show a patch.
GET /api/1.1/patches/1656/?format=api
{ "id": 1656, "url": "https://patchwork.libcamera.org/api/1.1/patches/1656/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1656/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190711125609.7331-2-laurent.pinchart@ideasonboard.com>", "date": "2019-07-11T12:56:09", "name": "[libcamera-devel,2/2] libcamera: signal: Fix Object handling in multiple inheritance cases", "commit_ref": "56c2e653008a5447eafc7509e6e2957470853495", "pull_url": null, "state": "accepted", "archived": false, "hash": "8d2963c3d445f048a2c70a87ef9da5be8c71a4c5", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/1656/mbox/", "series": [ { "id": 418, "url": "https://patchwork.libcamera.org/api/1.1/series/418/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=418", "date": "2019-07-11T12:56:08", "name": "[libcamera-devel,1/2] test: signal: Extend Signal test with multi-inheritance reeiver", "version": 1, "mbox": "https://patchwork.libcamera.org/series/418/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1656/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1656/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA01661574\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 11 Jul 2019 14:56:43 +0200 (CEST)", "from pendragon.ideasonboard.com (softbank126163157105.bbtec.net\n\t[126.163.157.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7963531C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 11 Jul 2019 14:56:42 +0200 (CEST)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562849803;\n\tbh=ygvYERfLsYBwdYMhEybRmX/HKMd2chZAHc/fVD7txAA=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=UnLLaQ24CRXujkj0c0CHfkoypXs2rKxyWlnCYhl1BwtFRqserC2cZ8cCKtylPAlad\n\tcsSEOZEUvdrocSrKSBFeYGSCw2kMhdk9tN3ThjsNmfOgQio+kQsDTBXTU8zXsUgqBP\n\tq1tfFAvCvK0l79OxPvkVE+nOlDtx6P9CbMQYQeU0=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Thu, 11 Jul 2019 15:56:09 +0300", "Message-Id": "<20190711125609.7331-2-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.21.0", "In-Reply-To": "<20190711125609.7331-1-laurent.pinchart@ideasonboard.com>", "References": "<20190711125609.7331-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 2/2] libcamera: signal: Fix Object\n\thandling in multiple inheritance cases", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Thu, 11 Jul 2019 12:56:44 -0000" }, "content": "The SlotBase implementation stores the receiver object pointer as a void\npointer internally. The pointer is then cast back to an Object pointer\nwhen the receiver object class derives from Object. When the receiver is\nan object that inherits from both the Object class and other classes,\nthe Object data members may not be stored at the beginning of the object\nmemory. The cast back to an Object pointer is thus incorrect.\n\nFix this by casting the receiver object pointer to an Object pointer\nwhere the type of the receiver object is known, and pass it along with\nthe receiver void pointer to the SlotBase class. The SlotBase class\nstores both pointers internally, and doesn't need the isObject_ field\nanymore as the same information is obtained from checking if the Object\npointer is null.\n\nTo avoid confusing the two pointers, use the same naming scheme through\nthe whole implementation: \"obj\" points to a receiver object as an\nunknown type, and \"object\" to the receiver object cast to an Object. The\nlatter is null when the receiver object doesn't inherit from the Object\nclass.\n\nTo further clarify the code, remove direct access to the SlotBase \"obj\"\nand \"object\" fields as much as possible. They are replaced by two new\nmethods :\n\n- SlotBase::disconnect() to disconnect a signal from the slot's receiver\n object\n- SlotBase::match() to test if an object pointer matches the slot\n\nThe match() method is a template method with a specialisation for the\nObject type, to compare either the obj or the object pointer depending\non the type of the parameter. This is required as the Object destructor\ncalls the SignalBase::disconnect() method for signal connected to the\nobject, and passes a pointer to Object to that method, while the actual\nobject may have a different address due to the issue explained above.\nThe pointer must thus be compared with the stored Object pointer in that\ncase, not to the pointer to the receiver object.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/object.h | 4 ++-\n include/libcamera/signal.h | 55 +++++++++++++++++++++-----------------\n src/libcamera/signal.cpp | 8 +++++-\n 3 files changed, 40 insertions(+), 27 deletions(-)", "diff": "diff --git a/include/libcamera/object.h b/include/libcamera/object.h\nindex d61dfb1ebaef..5c251a822d9a 100644\n--- a/include/libcamera/object.h\n+++ b/include/libcamera/object.h\n@@ -13,9 +13,10 @@\n namespace libcamera {\n \n class Message;\n-class SignalBase;\n template<typename... Args>\n class Signal;\n+class SignalBase;\n+class SlotBase;\n class Thread;\n \n class Object\n@@ -33,6 +34,7 @@ public:\n private:\n \ttemplate<typename... Args>\n \tfriend class Signal;\n+\tfriend class SlotBase;\n \tfriend class Thread;\n \n \tvoid connect(SignalBase *signal);\ndiff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\nindex 11ffb857a59c..d3be362637f8 100644\n--- a/include/libcamera/signal.h\n+++ b/include/libcamera/signal.h\n@@ -18,23 +18,28 @@ namespace libcamera {\n \n template<typename... Args>\n class Signal;\n+class SignalBase;\n \n class SlotBase\n {\n public:\n-\tSlotBase(void *obj, bool isObject)\n-\t\t: obj_(obj), isObject_(isObject) {}\n+\tSlotBase(void *obj, Object *object)\n+\t\t: obj_(obj), object_(object) {}\n \tvirtual ~SlotBase() {}\n \n-\tvoid *obj() { return obj_; }\n-\tbool isObject() const { return isObject_; }\n+\ttemplate<typename T>\n+\tbool match(T *obj) { return obj == obj_; }\n+\ttemplate<>\n+\tbool match(Object *object) { return object == object_; }\n+\n+\tvoid disconnect(SignalBase *signal);\n \n \tvoid activatePack(void *pack);\n \tvirtual void invokePack(void *pack) = 0;\n \n protected:\n \tvoid *obj_;\n-\tbool isObject_;\n+\tObject *object_;\n };\n \n template<typename... Args>\n@@ -71,8 +76,8 @@ private:\n \t}\n \n public:\n-\tSlotArgs(void *obj, bool isObject)\n-\t\t: SlotBase(obj, isObject) {}\n+\tSlotArgs(void *obj, Object *object)\n+\t\t: SlotBase(obj, object) {}\n \n \tvoid invokePack(void *pack) override\n \t{\n@@ -89,12 +94,12 @@ class SlotMember : public SlotArgs<Args...>\n public:\n \tusing PackType = std::tuple<typename std::remove_reference<Args>::type...>;\n \n-\tSlotMember(T *obj, bool isObject, void (T::*func)(Args...))\n-\t\t: SlotArgs<Args...>(obj, isObject), func_(func) {}\n+\tSlotMember(T *obj, Object *object, void (T::*func)(Args...))\n+\t\t: SlotArgs<Args...>(obj, object), func_(func) {}\n \n \tvoid activate(Args... args)\n \t{\n-\t\tif (this->isObject_)\n+\t\tif (this->object_)\n \t\t\tSlotBase::activatePack(new PackType{ args... });\n \t\telse\n \t\t\t(static_cast<T *>(this->obj_)->*func_)(args...);\n@@ -115,7 +120,7 @@ class SlotStatic : public SlotArgs<Args...>\n {\n public:\n \tSlotStatic(void (*func)(Args...))\n-\t\t: SlotArgs<Args...>(nullptr, false), func_(func) {}\n+\t\t: SlotArgs<Args...>(nullptr, nullptr), func_(func) {}\n \n \tvoid activate(Args... args) { (*func_)(args...); }\n \tvoid invoke(Args... args) {}\n@@ -129,11 +134,11 @@ class SignalBase\n {\n public:\n \ttemplate<typename T>\n-\tvoid disconnect(T *object)\n+\tvoid disconnect(T *obj)\n \t{\n \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n \t\t\tSlotBase *slot = *iter;\n-\t\t\tif (slot->obj() == object) {\n+\t\t\tif (slot->match(obj)) {\n \t\t\t\titer = slots_.erase(iter);\n \t\t\t\tdelete slot;\n \t\t\t} else {\n@@ -155,27 +160,27 @@ public:\n \t~Signal()\n \t{\n \t\tfor (SlotBase *slot : slots_) {\n-\t\t\tif (slot->isObject())\n-\t\t\t\tstatic_cast<Object *>(slot->obj())->disconnect(this);\n+\t\t\tslot->disconnect(this);\n \t\t\tdelete slot;\n \t\t}\n \t}\n \n #ifndef __DOXYGEN__\n \ttemplate<typename T, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>\n-\tvoid connect(T *object, void (T::*func)(Args...))\n+\tvoid connect(T *obj, void (T::*func)(Args...))\n \t{\n+\t\tObject *object = static_cast<Object *>(obj);\n \t\tobject->connect(this);\n-\t\tslots_.push_back(new SlotMember<T, Args...>(object, true, func));\n+\t\tslots_.push_back(new SlotMember<T, Args...>(obj, object, func));\n \t}\n \n \ttemplate<typename T, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>\n #else\n \ttemplate<typename T>\n #endif\n-\tvoid connect(T *object, void (T::*func)(Args...))\n+\tvoid connect(T *obj, void (T::*func)(Args...))\n \t{\n-\t\tslots_.push_back(new SlotMember<T, Args...>(object, false, func));\n+\t\tslots_.push_back(new SlotMember<T, Args...>(obj, nullptr, func));\n \t}\n \n \tvoid connect(void (*func)(Args...))\n@@ -191,23 +196,23 @@ public:\n \t}\n \n \ttemplate<typename T>\n-\tvoid disconnect(T *object)\n+\tvoid disconnect(T *obj)\n \t{\n-\t\tSignalBase::disconnect(object);\n+\t\tSignalBase::disconnect(obj);\n \t}\n \n \ttemplate<typename T>\n-\tvoid disconnect(T *object, void (T::*func)(Args...))\n+\tvoid disconnect(T *obj, void (T::*func)(Args...))\n \t{\n \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n \t\t\tSlotArgs<Args...> *slot = static_cast<SlotArgs<Args...> *>(*iter);\n \t\t\t/*\n-\t\t\t * If the obj() pointer matches the object, the slot is\n+\t\t\t * If the object matches the slot, the slot is\n \t\t\t * guaranteed to be a member slot, so we can safely\n \t\t\t * cast it to SlotMember<T, Args...> and access its\n \t\t\t * func_ member.\n \t\t\t */\n-\t\t\tif (slot->obj() == object &&\n+\t\t\tif (slot->match(obj) &&\n \t\t\t static_cast<SlotMember<T, Args...> *>(slot)->func_ == func) {\n \t\t\t\titer = slots_.erase(iter);\n \t\t\t\tdelete slot;\n@@ -221,7 +226,7 @@ public:\n \t{\n \t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n \t\t\tSlotArgs<Args...> *slot = *iter;\n-\t\t\tif (slot->obj() == nullptr &&\n+\t\t\tif (slot->match(nullptr) &&\n \t\t\t static_cast<SlotStatic<Args...> *>(slot)->func_ == func) {\n \t\t\t\titer = slots_.erase(iter);\n \t\t\t\tdelete slot;\ndiff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\nindex 53c18535fee3..ab7dba508dfb 100644\n--- a/src/libcamera/signal.cpp\n+++ b/src/libcamera/signal.cpp\n@@ -57,9 +57,15 @@ namespace libcamera {\n * passed through the signal will remain valid after the signal is emitted.\n */\n \n+void SlotBase::disconnect(SignalBase *signal)\n+{\n+\tif (object_)\n+\t\tobject_->disconnect(signal);\n+}\n+\n void SlotBase::activatePack(void *pack)\n {\n-\tObject *obj = static_cast<Object *>(obj_);\n+\tObject *obj = static_cast<Object *>(object_);\n \n \tif (Thread::current() == obj->thread()) {\n \t\tinvokePack(pack);\n", "prefixes": [ "libcamera-devel", "2/2" ] }