[{"id":26902,"web_url":"https://patchwork.libcamera.org/comment/26902/","msgid":"<20230419060425.GA9370@pendragon.ideasonboard.com>","date":"2023-04-19T06:04:25","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: device_enumerator_udev:\n\tUse std::string_view","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Apr 18, 2023 at 04:26:04PM +0000, Barnabás Pőcze via libcamera-devel wrote:\n> In `udevNotify()`, constructing an std::string from the device's\n> associated action is unnecessary as it is only compared against\n> static strings, and for that purpose an std::string_view works\n> just as well, while being cheaper to construct.\n> \n> In the same vein, an std::string_view can be used to\n> store the device's devnode initially, and the string\n> construction can be deferred until it is needed.\n> \n> Furthermore, previously `udev_device_get_devnode()` was\n> called twice. The extra call is now removed.\n> \n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/device_enumerator_udev.cpp | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> index a63cd360..0abc1248 100644\n> --- a/src/libcamera/device_enumerator_udev.cpp\n> +++ b/src/libcamera/device_enumerator_udev.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -331,18 +332,18 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)\n> void DeviceEnumeratorUdev::udevNotify()\n> {\n> \tstruct udev_device *dev = udev_monitor_receive_device(monitor_);\n> -\tstd::string action(udev_device_get_action(dev));\n> -\tstd::string deviceNode(udev_device_get_devnode(dev));\n> +\tstd::string_view action(udev_device_get_action(dev));\n> +\tstd::string_view deviceNode(udev_device_get_devnode(dev));\n> \n> \tLOG(DeviceEnumerator, Debug)\n> -\t\t<< action << \" device \" << udev_device_get_devnode(dev);\n> +\t\t<< action << \" device \" << deviceNode;\n> \n> \tif (action == \"add\") {\n> \t\taddUdevDevice(dev);\n> \t} else if (action == \"remove\") {\n> \t\tconst char *subsystem = udev_device_get_subsystem(dev);\n> \t\tif (subsystem && !strcmp(subsystem, \"media\"))\n> -\t\t\tremoveDevice(deviceNode);\n> +\t\t\tremoveDevice(std::string(deviceNode));\n> \t}\n> \n> \tudev_device_unref(dev);","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 89EBFBE173\n\tfor ;\n\tWed, 19 Apr 2023 06:04:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0B53627B1;\n\tWed, 19 Apr 2023 08:04:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 088CF603A0\n\tfor ;\n\tWed, 19 Apr 2023 08:04:14 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B68512F;\n\tWed, 19 Apr 2023 08:04:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1681884255;\n\tbh=Z7fiUhoEI8PzjP40CYfLMMRXKLC1sWqbjsbNJJk6yv0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cBYAjYAUXk59sI284l0xZI/GYXT1+ocmUKc6gT9nhJt6bADy4UGahC5GfBqzJ7pwr\n\t12N1PXNwkID11O1moXGqMeXOJdu2OKufLeNMJ7/4ZJQgVQBpsZbhIVoF1Q4Yh3kVGu\n\tO9fJyx78ZUhQclaWz9VWiQYIfOI2nAcO4StFLUMtabIPtbhJatf67QzWPoFI/0hX9E\n\tkWqjxy8skWjfxPbFeOfB1s7rvTxJeeIk9Uo3nUHPNUBAem7bJtDWv0GhXur7E3KKo4\n\tikYMzjqE/uEhFJrSXBq/B5Jye5oM/skA4VDLfEviBHRq06Ed0A71doKCa145oStsM1\n\tS1Fen6l3Gd3Eg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1681884247;\n\tbh=Z7fiUhoEI8PzjP40CYfLMMRXKLC1sWqbjsbNJJk6yv0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fYbcxu0De0Grgughc4Z5DcoedrOk9TJt5wmvfdBexneZyevma/yRtylNldGJVPjDU\n\tUuprEqSrAkCsCJi2K1AY1ubznI2iI7mpuJcGnT0zN6T+h0zzJ4AcRgZ+EU4mt3MdAH\n\tUHl7lw98SXYuXKzc37nDXk26jlegpTLPoV8FHRDQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fYbcxu0D\"; dkim-atps=neutral","Date":"Wed, 19 Apr 2023 09:04:25 +0300","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Message-ID":"<20230419060425.GA9370@pendragon.ideasonboard.com>","References":"<20230418162603.449433-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20230418162603.449433-1-pobrn@protonmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: device_enumerator_udev:\n\tUse std::string_view","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26903,"web_url":"https://patchwork.libcamera.org/comment/26903/","msgid":"","date":"2023-04-19T07:13:52","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: device_enumerator_udev:\n\tUse std::string_view","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, Apr 18, 2023 at 04:26:04PM +0000, Barnabás Pőcze via libcamera-devel wrote:\n> In `udevNotify()`, constructing an std::string from the device's\n> associated action is unnecessary as it is only compared against\n> static strings, and for that purpose an std::string_view works\n> just as well, while being cheaper to construct.\n>\n> In the same vein, an std::string_view can be used to\n> store the device's devnode initially, and the string\n> construction can be deferred until it is needed.\n>\n> Furthermore, previously `udev_device_get_devnode()` was\n> called twice. The extra call is now removed.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/device_enumerator_udev.cpp | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> index a63cd360..0abc1248 100644\n> --- a/src/libcamera/device_enumerator_udev.cpp\n> +++ b/src/libcamera/device_enumerator_udev.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -331,18 +332,18 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)\n> void DeviceEnumeratorUdev::udevNotify()\n> {\n> \tstruct udev_device *dev = udev_monitor_receive_device(monitor_);\n> -\tstd::string action(udev_device_get_action(dev));\n> -\tstd::string deviceNode(udev_device_get_devnode(dev));\n> +\tstd::string_view action(udev_device_get_action(dev));\n> +\tstd::string_view deviceNode(udev_device_get_devnode(dev));\n>\n> \tLOG(DeviceEnumerator, Debug)\n> -\t\t<< action << \" device \" << udev_device_get_devnode(dev);\n> +\t\t<< action << \" device \" << deviceNode;\n>\n> \tif (action == \"add\") {\n> \t\taddUdevDevice(dev);\n> \t} else if (action == \"remove\") {\n> \t\tconst char *subsystem = udev_device_get_subsystem(dev);\n> \t\tif (subsystem && !strcmp(subsystem, \"media\"))\n> -\t\t\tremoveDevice(deviceNode);\n> +\t\t\tremoveDevice(std::string(deviceNode));\n\nNot sure what the actual gain actually is, if we have to construct a\nstring here, but ok...\n\nWrapping in a string_view the values returned by\nudev_device_get_devnode() and udev_device_get_action() seems safe as\naccording to `man 3 udev_device_get_devnode`:\n\n return a pointer to a constant string that describes the requested\n property. The lifetime of this string is bound to the device it was\n requested on\n\nso\n\nReviewed-by: Jacopo Mondi \n\n\n\n> \t}\n>\n> \tudev_device_unref(dev);\n> --\n> 2.40.0\n>\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 EF189BD1F1\n\tfor ;\n\tWed, 19 Apr 2023 07:13:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F337627AB;\n\tWed, 19 Apr 2023 09:13:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2CA3627AB\n\tfor ;\n\tWed, 19 Apr 2023 09:13:55 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5551C10A;\n\tWed, 19 Apr 2023 09:13:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1681888437;\n\tbh=/WPY+bMJBBDJVGJeSxufdVsLR6AQRmVyPYlSn1flcFo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0u4Rm4eF62CI6WdCx0Pp5ouFKXeYaF3Yl+DuV4KpRs6v/vPT76BPGc8pZ9RbXjuHr\n\tO8lQiv+Ga61SsanWndlWWbTvaWe994MMqo5eDfKvaVd+4JHJXVnkLtjX/D327JkJoC\n\tPJFWWbpIkLAeCnJS7bYnlsLoCC1B4cMHnlbRRhhwafF0SxdG0yh5qoN7UvdtdsZPoP\n\tKw6C1W8OG9REqmfh3Ss4AST1oT350pgBMn83LCd0xj9eL3xKfHi2m2m+c7KE7lgIJ+\n\tsQiczjV7g53CoZ6VQVKS0cHYevkSO5tqbjwJwv7K1crV10JT9oYXrP6wvbMHjimYK+\n\tPvFmhW4SJhnLg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1681888429;\n\tbh=/WPY+bMJBBDJVGJeSxufdVsLR6AQRmVyPYlSn1flcFo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TTQPIrMhv5eeRhrR+aumF6Xx8a8OGe4aWXJTW4vyZRi4rfc1GE8voufo1paGHmsxh\n\tRykfamlTHz+Ztvp3wmW05HAJcKk1+ScA9wL/6sYZb5v2BPMpMOX9F6a+7TKW7NvlCa\n\tgOZieI7668C31t9nIJA5Aw0C79Taxt0N1U2nVcc0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TTQPIrMh\"; dkim-atps=neutral","Date":"Wed, 19 Apr 2023 09:13:52 +0200","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Message-ID":"","References":"<20230418162603.449433-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20230418162603.449433-1-pobrn@protonmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: device_enumerator_udev:\n\tUse std::string_view","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]