[{"id":32274,"web_url":"https://patchwork.libcamera.org/comment/32274/","msgid":"<20241119113058.GK31681@pendragon.ideasonboard.com>","date":"2024-11-19T11:30:58","subject":"Re: [PATCH v2 4/9] libcamera: internal: matrix: Replace vector with\n\tarray in constructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Tue, Nov 19, 2024 at 11:37:31AM +0100, Stefan Klug wrote:\n> Having an array based constructor gives us initialization via\n> initializer lists.\n\nThis needs a bit more information.\n\n\nThe Matrix constructor that takes a std::vector is meant and only used\nto initialize a Matrix from an initializer list. Using a std::vector is\nproblematic for two reasons. First, it requires constructing a vector,\ncopying the data from the initializer list, which is an expensive\noperation. Then, the vector size can't be verified at compile time,\nmaking the constructor unsafe.\n\nThe first issue could be solved by replacing the vector with a\nstd::initializer_list or a Span. The second issue would require checking\nthe initializer list size with a static assertion, or restricting usage\nof the constructor to fixed-extent spans. Unfortunately, even if the\nsize of initializer lists is always known at compile time, the\nstd::initializer_list::size() function is a compile-time constant only\nfor constant initializer lists. Using a span would work better, but\nconstruction of a fixed extent span from an initializer list must be\nexplicit, making the API cumbersome.\n\nWe can solve all those issues by passing an std::array to the\nconstructor. Construction of an array from an initializer list can be\nimplicit and doesn't involve a copy, and the array size is a template\nparameter and therefore guaranteed to be a compile-time constant.\n\n> Signed-off-by: Stefan Klug \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> include/libcamera/internal/matrix.h | 2 +-\n> src/libcamera/matrix.cpp | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h\n> index 3701d0ee980b..7a71028c473a 100644\n> --- a/include/libcamera/internal/matrix.h\n> +++ b/include/libcamera/internal/matrix.h\n> @@ -33,7 +33,7 @@ public:\n> \t\tdata_.fill(static_cast(0));\n> \t}\n> \n> -\tMatrix(const std::vector &data)\n> +\tMatrix(const std::array &data)\n> \t{\n> \t\tstd::copy(data.begin(), data.end(), data_.begin());\n> \t}\n> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp\n> index 55359aa206ee..4d95a19bfbb9 100644\n> --- a/src/libcamera/matrix.cpp\n> +++ b/src/libcamera/matrix.cpp\n> @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(Matrix)\n> */\n> \n> /**\n> - * \\fn Matrix::Matrix(const std::vector &data)\n> + * \\fn Matrix::Matrix(const std::array &data)\n> * \\brief Construct a matrix from supplied data\n> * \\param[in] data Data from which to construct a matrix\n> *","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 810FBC32F1\n\tfor ;\n\tTue, 19 Nov 2024 11:31:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4997E65F04;\n\tTue, 19 Nov 2024 12:31:10 +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 6FAD6658A2\n\tfor ;\n\tTue, 19 Nov 2024 12:31:08 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 167CAD49;\n\tTue, 19 Nov 2024 12:30:51 +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=\"Wh/x6N6B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732015851;\n\tbh=hzhu61mOGdfwEvggBSA0NW6XiOPGVdsYvwboRZxfheA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wh/x6N6B9P289d73YDpgKNo4cakc8ybKyd9alzazLgY2jDgo5WR8RpPzEja614/8s\n\tOcaLeGIRkimmlgGYglSBgODboNK/200eYxTOCF6nEtmW94VjFyym974qf1sHUuD8SW\n\t7/d567I82aLncM4lsq37HWA4df/Xg7bRM3hI13Hg=","Date":"Tue, 19 Nov 2024 13:30:58 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 4/9] libcamera: internal: matrix: Replace vector with\n\tarray in constructor","Message-ID":"<20241119113058.GK31681@pendragon.ideasonboard.com>","References":"<20241119103740.1919807-1-stefan.klug@ideasonboard.com>\n\t<20241119103740.1919807-5-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241119103740.1919807-5-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]