[libcamera-devel,v3,2/2] libcamera: base: object: Prevent the same signal being connected more than once

15292 diff mbox series

Objects shouldn't be connected to the same signal more than once. Doing
so indicates a bug in the code, and can be highlighted in debug builds
with an assert that performs a lookup on the signals_ list.

Remove the support in the test framework which uses multiple Signal
connections on the same object.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---

v2:
 - Move assertion / validation to object instead of signal.
 - This equivalent list assertion in signal does not trap the reported
   issue, while this does validate and trap on the reported bug.

 src/libcamera/base/object.cpp | 6 ++++++
 test/signal.cpp               | 8 ++------
 2 files changed, 8 insertions(+), 6 deletions(-)

Hi Kieran,

Thank you for the patch.

On Fri, Jan 21, 2022 at 02:24:20PM +0000, Kieran Bingham wrote:
> Objects shouldn't be connected to the same signal more than once. Doing
> so indicates a bug in the code, and can be highlighted in debug builds
> with an assert that performs a lookup on the signals_ list.

There *could* be valid use cases for connecting a signal to the same
slot multiple times, although I have a hard time thinking of one. If
libcamera-base was meant to be used as a fully generic C++ helper
library, I'd be a bit concerned about this limitation, but for our use
cases, I think it's acceptable. I would however like to document this
change as such: a compromise between features and defensive programming
(here and in the comment in Object::connect()).

There's also a statement in the Signal class documentation that states
"Duplicate connections between a signal and a slot are allowed and
result in the slot being called multiple times for the same signal
emission", which isn't true anymore. It should be updated to document
the new forbidden behaviour (which may be a bit tricky to express nicely
without documenting implementation details in the API).

> Remove the support in the test framework which uses multiple Signal
> connections on the same object.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 
> v2:
>  - Move assertion / validation to object instead of signal.
>  - This equivalent list assertion in signal does not trap the reported
>    issue, while this does validate and trap on the reported bug.
> 
>  src/libcamera/base/object.cpp | 6 ++++++
>  test/signal.cpp               | 8 ++------
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp
> index 3f28768e48f8..7311b1d5a3a1 100644
> --- a/src/libcamera/base/object.cpp
> +++ b/src/libcamera/base/object.cpp
> @@ -284,6 +284,12 @@ void Object::notifyThreadMove()
>  
>  void Object::connect(SignalBase *signal)
>  {
> +	/*
> +	 * Connecting the same signal to an object multiple times is not
> +	 * supported.
> +	 */
> +	ASSERT(signals_.end() == std::find(signals_.begin(), signals_.end(), signal));

Don't we usually write it the other way around ?

	ASSERT(std::find(signals_.begin(), signals_.end(), signal) == signals_.end());

> +
>  	signals_.push_back(signal);
>  }
>  
> diff --git a/test/signal.cpp b/test/signal.cpp
> index fcf2def18df4..a1effaab0346 100644
> --- a/test/signal.cpp
> +++ b/test/signal.cpp
> @@ -212,14 +212,12 @@ protected:
>  		/* ----------------- Signal -> Object tests ----------------- */
>  
>  		/*
> -		 * Test automatic disconnection on object deletion. Connect the
> -		 * slot twice to ensure all instances are disconnected.
> +		 * Test automatic disconnection on object deletion.
>  		 */
>  		signalVoid_.disconnect();
>  
>  		SlotObject *slotObject = new SlotObject();
>  		signalVoid_.connect(slotObject, &SlotObject::slot);
> -		signalVoid_.connect(slotObject, &SlotObject::slot);

Could we connect two different signals to the slot, to test that all
signals are disconnected ? That will require emitting the second signal
a few lines below too.

>  		delete slotObject;
>  		valueStatic_ = 0;
>  		signalVoid_.emit();
> @@ -298,14 +296,12 @@ protected:
>  		/* --------- Signal -> Object (multiple inheritance) -------- */
>  
>  		/*
> -		 * Test automatic disconnection on object deletion. Connect the
> -		 * slot twice to ensure all instances are disconnected.
> +		 * Test automatic disconnection on object deletion.
>  		 */
>  		signalVoid_.disconnect();
>  
>  		SlotMulti *slotMulti = new SlotMulti();
>  		signalVoid_.connect(slotMulti, &SlotMulti::slot);
> -		signalVoid_.connect(slotMulti, &SlotMulti::slot);

Same here.

>  		delete slotMulti;
>  		valueStatic_ = 0;
>  		signalVoid_.emit();