From patchwork Mon Feb 22 11:15:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 11362 X-Patchwork-Delegate: laurent.pinchart@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 31AA4BD1F6 for ; Mon, 22 Feb 2021 11:16:03 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 911A168A04; Mon, 22 Feb 2021 12:16:02 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="lVxH4tfN"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 17435637DA for ; Mon, 22 Feb 2021 12:16:01 +0100 (CET) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B1BD36B5; Mon, 22 Feb 2021 12:16:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1613992560; bh=C8j6BEt5RObCy9Qwbi9q3ZIrHzyI0XIRHb6/GIXwxBQ=; h=From:To:Cc:Subject:Date:From; b=lVxH4tfNinFkDa5fvefYsj6hWK3XeuJkU4SNMcU0KpULVUq04SPhjWjoDcEgrLrY+ B5rYtCbxaSZ3jfzgI+kvdA13j8lX1ga0baKVgoCh//LSz4L9KUrTdDrth+oohwReok taDEPeK0Hb2jE8d0giK+EBctYF90TWTCcvbFxybo= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Mon, 22 Feb 2021 13:15:31 +0200 Message-Id: <20210222111531.25845-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.28.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH/RFC] libcamera: controls: Disable const move constructor and assignement X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 --- 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