[{"id":22091,"web_url":"https://patchwork.libcamera.org/comment/22091/","msgid":"","date":"2022-02-01T10:21:58","subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: base: object:\n\tPrevent the same signal being connected more than once","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Jan 21, 2022 at 02:24:20PM +0000, Kieran Bingham wrote:\n> Objects shouldn't be connected to the same signal more than once. Doing\n> so indicates a bug in the code, and can be highlighted in debug builds\n> with an assert that performs a lookup on the signals_ list.\n\nThere *could* be valid use cases for connecting a signal to the same\nslot multiple times, although I have a hard time thinking of one. If\nlibcamera-base was meant to be used as a fully generic C++ helper\nlibrary, I'd be a bit concerned about this limitation, but for our use\ncases, I think it's acceptable. I would however like to document this\nchange as such: a compromise between features and defensive programming\n(here and in the comment in Object::connect()).\n\nThere's also a statement in the Signal class documentation that states\n\"Duplicate connections between a signal and a slot are allowed and\nresult in the slot being called multiple times for the same signal\nemission\", which isn't true anymore. It should be updated to document\nthe new forbidden behaviour (which may be a bit tricky to express nicely\nwithout documenting implementation details in the API).\n\n> Remove the support in the test framework which uses multiple Signal\n> connections on the same object.\n> \n> Signed-off-by: Kieran Bingham \n> Reviewed-by: Jacopo Mondi \n> ---\n> \n> v2:\n> - Move assertion / validation to object instead of signal.\n> - This equivalent list assertion in signal does not trap the reported\n> issue, while this does validate and trap on the reported bug.\n> \n> src/libcamera/base/object.cpp | 6 ++++++\n> test/signal.cpp | 8 ++------\n> 2 files changed, 8 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp\n> index 3f28768e48f8..7311b1d5a3a1 100644\n> --- a/src/libcamera/base/object.cpp\n> +++ b/src/libcamera/base/object.cpp\n> @@ -284,6 +284,12 @@ void Object::notifyThreadMove()\n> \n> void Object::connect(SignalBase *signal)\n> {\n> +\t/*\n> +\t * Connecting the same signal to an object multiple times is not\n> +\t * supported.\n> +\t */\n> +\tASSERT(signals_.end() == std::find(signals_.begin(), signals_.end(), signal));\n\nDon't we usually write it the other way around ?\n\n\tASSERT(std::find(signals_.begin(), signals_.end(), signal) == signals_.end());\n\n> +\n> \tsignals_.push_back(signal);\n> }\n> \n> diff --git a/test/signal.cpp b/test/signal.cpp\n> index fcf2def18df4..a1effaab0346 100644\n> --- a/test/signal.cpp\n> +++ b/test/signal.cpp\n> @@ -212,14 +212,12 @@ protected:\n> \t\t/* ----------------- Signal -> Object tests ----------------- */\n> \n> \t\t/*\n> -\t\t * Test automatic disconnection on object deletion. Connect the\n> -\t\t * slot twice to ensure all instances are disconnected.\n> +\t\t * Test automatic disconnection on object deletion.\n> \t\t */\n> \t\tsignalVoid_.disconnect();\n> \n> \t\tSlotObject *slotObject = new SlotObject();\n> \t\tsignalVoid_.connect(slotObject, &SlotObject::slot);\n> -\t\tsignalVoid_.connect(slotObject, &SlotObject::slot);\n\nCould we connect two different signals to the slot, to test that all\nsignals are disconnected ? That will require emitting the second signal\na few lines below too.\n\n> \t\tdelete slotObject;\n> \t\tvalueStatic_ = 0;\n> \t\tsignalVoid_.emit();\n> @@ -298,14 +296,12 @@ protected:\n> \t\t/* --------- Signal -> Object (multiple inheritance) -------- */\n> \n> \t\t/*\n> -\t\t * Test automatic disconnection on object deletion. Connect the\n> -\t\t * slot twice to ensure all instances are disconnected.\n> +\t\t * Test automatic disconnection on object deletion.\n> \t\t */\n> \t\tsignalVoid_.disconnect();\n> \n> \t\tSlotMulti *slotMulti = new SlotMulti();\n> \t\tsignalVoid_.connect(slotMulti, &SlotMulti::slot);\n> -\t\tsignalVoid_.connect(slotMulti, &SlotMulti::slot);\n\nSame here.\n\n> \t\tdelete slotMulti;\n> \t\tvalueStatic_ = 0;\n> \t\tsignalVoid_.emit();","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 281A6BF415\n\tfor ;\n\tTue, 1 Feb 2022 10:22:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5EF58609DF;\n\tTue, 1 Feb 2022 11:22:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D71B6609AF\n\tfor ;\n\tTue, 1 Feb 2022 11:22:21 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47A06332;\n\tTue, 1 Feb 2022 11:22:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"K8iJxmL4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643710941;\n\tbh=wLWuO0l+2tMzPJs8SSxPW7Suh+IH+4IeZb2sKd3uh+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K8iJxmL46Vi8Yva6u3AY1LMdmCLPXe4ocBiCYqXgkOiUHSqDvK3LGDs+/Ms8TJbZS\n\ti7exwRPj7YcDQj7R9J3k/rbZxMayHOB6RpEtUspY4G7FvlheIYZ/HBg1Sow238I66F\n\tZPKRE2gv0r7VHahOaUZokoif2oWjQ8iNlI7ZkYtw=","Date":"Tue, 1 Feb 2022 12:21:58 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20220121142420.3531497-1-kieran.bingham@ideasonboard.com>\n\t<20220121142420.3531497-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220121142420.3531497-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: base: object:\n\tPrevent the same signal being connected more than once","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]