[libcamera-devel] [PATCH/RFC] libcamera: controls: Disable const move constructor and assignement

[Laurent Pinchart]

C++ supports const rvalue references. They are of dubious use, as the
purpose of rvalue references is to express move semantics, and moving a
const object is not possible, but they nonetheless exist. This means
that calling std::move() on a const reference will not produce an error,
but return a const rvalue reference.

Furthermore, a const lvalue reference can bind to a const rvalue. This
means that overload resolution will select an overload of a function
that takes a const lvalue reference (such as the copy constructor) when
passed a const rvalue reference (such as the return value of std::move()
called on a const reference) if no overload taking a const rvalue
reference exists.

The consequence of this is that the following code

	const ControlList &ctrls = ...;
	...
	ControlList copy(std::move(ctrls));

will select the ControlList copy constructor and thus compile, when one
may expect the object to be moved from the use of std::move() (there is,
on a side note, no guarantee in the language that a move constructor
will move anything).

To make the compiler reject usage of std::move() in this case, define
the copy and move constructor and assignment operators explicitly,
defaulting the non-const version of the move constructor and assignment
operator, but deleting the const version. The compiler will select the
const move version as it is declared, and generate an error as it is
deleted.

Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
 include/libcamera/controls.h |  8 ++++++++
 src/libcamera/controls.cpp   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

I'm sending this as an RFC as I'm not sure such an explicit approach is
worth it. We'd have to apply it to all classes that can be copied and
moved, which will generate quite a bit of churn (especially in the
documentation, although we could surround the functions in the header
with an #ifndef __DOXYGEN__). Maybe a macro would make sense, something
along the lines of

#define LIBCAMERA_COPYABLE_MOVEABLE(klass)              \
	klass(const klass &other) = default;            \
	klass(klass &&other) = default;                 \
	klass(const klass &&other) = delete;            \
	klass &operator=(const klass &other) = default; \
	klass &operator=(klass &&other) = default;      \
	klass &operator=(const klass &&other) = delete;

(that would however not work with #ifndef __DOXYGEN__, but it may be
possible to create a macro for the documentation too).

There's of course also the possibility that I'm missing something, and
that the silent use of the copy constructor when called with a const
rvalue is actually a useful feature.

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 1a5690a5ccbe..fe4b159eb172 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -353,6 +353,14 @@ public:
 	ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
 	ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
 
+	ControlList(const ControlList &other) = default;
+	ControlList(ControlList &&other) = default;
+	ControlList(const ControlList &&other) = delete;
+
+	ControlList &operator=(const ControlList &other) = default;
+	ControlList &operator=(ControlList &&other) = default;
+	ControlList &operator=(const ControlList &&other) = delete;
+
 	using iterator = ControlListMap::iterator;
 	using const_iterator = ControlListMap::const_iterator;
 
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index c58ed3946f3b..53957322c505 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -821,6 +821,36 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida
 {
 }
 
+/**
+ * \fn ControlList::ControlList(const ControlList &other)
+ * \brief Copy constructor, construct a ControlList by copying \a other
+ * \param[in] other The ControlList to copy content from
+ */
+
+/**
+ * \fn ControlList::ControlList(ControlList &&other)
+ * \brief Move constructor, construct a ControlList by moving \a other
+ * \param[in] other The ControlList to move content from
+ *
+ * Upon return the \a other ControlList is empty.
+ */
+
+/**
+ * \fn ControlList::operator=(const ControlList &other)
+ * \brief Copy assignment operator, replace the content of the ControlList with
+ * a copy of \a other
+ * \param[in] other The ControlList to copy content from
+ */
+
+/**
+ * \fn ControlList::operator=(ControlList &&other)
+ * \brief Move assignment operator, replace the content of the ControlList with
+ * the content of \a other
+ * \param[in] other The ControlList to move content from
+ *
+ * Upon return the \a other ControlList is empty.
+ */
+
 /**
  * \typedef ControlList::iterator
  * \brief Iterator for the controls contained within the list
-- 
Regards,

Laurent Pinchart