[{"id":15964,"web_url":"https://patchwork.libcamera.org/comment/15964/","msgid":"","date":"2021-03-28T05:42:07","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","submitter":{"id":62,"url":"https://patchwork.libcamera.org/api/people/62/","name":"Khem Raj","email":"raj.khem@gmail.com"},"content":"Thanks Laurent, for this, I know it was on my plate to address your\nfeedback but I could not get to it thus far\n\nOn Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart\n wrote:\n>\n> From: Khem Raj \n>\n> A range-based for loop whose range expression is an array of char\n> pointers and range variable declaration is a const reference to a\n> std::string creates a temporary string from the char pointer and binds\n> the range variable reference to it. This creates a const reference to a\n> temporary, which is valid in C++, and extends the lifetime of the\n> temporary to the lifetime of the reference.\n>\n> However, lifetime extension in range-based for loops is considered as a\n> sign of a potential issue, as a temporary is created for every\n> iteration, which can be costly, and the usage of a reference in the\n> range declaration doesn't make it obvious that the code isn't simply\n> binding a reference to an existing object. gcc 11, with the\n> -Wrange-loop-construct option, flags this:\n>\n> uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]\n> | 432 | for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> | | ^~~~\n>\n> To please the compiler, make the range variable a const char *. This may\n> bring a tiny performance improvement, as the name is only used once, in\n> a location where the compiler can use\n>\n> operator+(const std::string &, const char *)\n>\n> instead of\n>\n> operator+(const std::string &, const std::string &)\n>\n> Signed-off-by: Khem Raj \n> Reviewed-by: Laurent Pinchart \n>\n> Use a const char * type instead of auto, and update the commit message\n> accordingly.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 031f96e28449..b6c6ade5ebaf 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)\n>\n> /* Creata a device ID from the USB devices vendor and product ID. */\n> std::string deviceId;\n> - for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> + for (const char *name : { \"idVendor\", \"idProduct\" }) {\n> std::ifstream file(path + \"/../\" + name);\n>\n> if (!file.is_open())\n> --\n> Regards,\n>\n> Laurent Pinchart\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 E4181C32EA\n\tfor ;\n\tSun, 28 Mar 2021 05:42:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18B8268781;\n\tSun, 28 Mar 2021 07:42:37 +0200 (CEST)","from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com\n\t[IPv6:2607:f8b0:4864:20::72b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B2F56051B\n\tfor ;\n\tSun, 28 Mar 2021 07:42:35 +0200 (CEST)","by mail-qk1-x72b.google.com with SMTP id y18so9406417qky.11\n\tfor ;\n\tSat, 27 Mar 2021 22:42:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"CXrUwrJh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oQssEgnnXpxW9iRml7BuqGHAO/Mxyu2jWktEyETNJ20=;\n\tb=CXrUwrJhBhkiLhv09EFs5ur5Ox/npimMoDuC0q9Mve3T0FlPw/vG7yKdzEs2VPhMtM\n\tK55QDbQsbuDalG2vmStf2I8YfM18yHbUKCPK9Ev6Y/HKL59BBMKhyafnL/6cpvg7K/5W\n\te0+Ok3Z0mIDEErtRBRAVa8SQhAMq5Lf3G9gA2kGqe4oTdw0f2X1uGQ4taZ7slt7Yjv/I\n\tmp2zOFW5OTemUuf7TDDmaqs2IN7Eprnk4XeJ8CWoINrtx70vE8u8PssnCM25Sr8FX/fL\n\tMMdWdEoHMwd6oRpFuEM7Q6Tcsft01IQ78oA7+AN7mj36QqPy3VrVvrPlaQfWjGsN4xjE\n\tLCaQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oQssEgnnXpxW9iRml7BuqGHAO/Mxyu2jWktEyETNJ20=;\n\tb=oMsl/SZ9IaGVuvl3atZKOry/c3Hk/iYYFKNIc/8MzWOeDRfjPBOzvEPhz3cOGg8rZc\n\tJZOimKJodrCmKCm7D0qqVILGiHWyZaBrlFaC3rTE4cIJS1FweObdqPw5HDPrKuzEf+cH\n\tCdWKErZAQX/WGkJtcXoK3DOurYCxBbCPTXhgpdbftPpjZ/rBxB5ZxF3Pv1Ls5CDeIwa1\n\t3o8DmSSL7L9bhl3sOpdZmXwCRvmAM15UrxPUIhcyXKUEwjwe9hHEz6+023yDpbPpKexB\n\t64Fg10Qjmi/HdOexAXYgRtCqF1X7OT9Jv7AjS/PnCu4+cn9TxysJKdLOvKUuqkj5KV2x\n\ts5RQ==","X-Gm-Message-State":"AOAM533uH3+LVi+yC5v0WNCw3ktQAlXHr7APq3AO27+nb5/29qmD2ZUh\n\t0z0rG8NrZ5EFRJRaKUiY6iGUBNbHQT1anbSV+Os=","X-Google-Smtp-Source":"ABdhPJyzG0w8qsDHTztEqbu7iXJ2GdIWh0TRPcyjYsYVmXNPGxrUjSRTy9EFSScANBDBI8aU+F+XDdbYMCpD5Eyz/9M=","X-Received":"by 2002:a37:a350:: with SMTP id\n\tm77mr19589513qke.146.1616910153670; \n\tSat, 27 Mar 2021 22:42:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>","From":"Khem Raj ","Date":"Sat, 27 Mar 2021 22:42:07 -0700","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15968,"web_url":"https://patchwork.libcamera.org/comment/15968/","msgid":"","date":"2021-03-28T16:55:28","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Khem,\n\nOn Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:\n> Thanks Laurent, for this, I know it was on my plate to address your\n> feedback but I could not get to it thus far\n\nNo worries. I was investigating this issue, to have a better\nunderstanding of why the lifetime extension was a problem, and have\nreworded the commit message as a result, so I thought it was worth\nposting a v2. I'd appreciate if you could test with gcc-11 when you'll\nhave a chance, but there's no urgency.\n\n> On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:\n> >\n> > From: Khem Raj \n> >\n> > A range-based for loop whose range expression is an array of char\n> > pointers and range variable declaration is a const reference to a\n> > std::string creates a temporary string from the char pointer and binds\n> > the range variable reference to it. This creates a const reference to a\n> > temporary, which is valid in C++, and extends the lifetime of the\n> > temporary to the lifetime of the reference.\n> >\n> > However, lifetime extension in range-based for loops is considered as a\n> > sign of a potential issue, as a temporary is created for every\n> > iteration, which can be costly, and the usage of a reference in the\n> > range declaration doesn't make it obvious that the code isn't simply\n> > binding a reference to an existing object. gcc 11, with the\n> > -Wrange-loop-construct option, flags this:\n> >\n> > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]\n> > | 432 | for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > | | ^~~~\n> >\n> > To please the compiler, make the range variable a const char *. This may\n> > bring a tiny performance improvement, as the name is only used once, in\n> > a location where the compiler can use\n> >\n> > operator+(const std::string &, const char *)\n> >\n> > instead of\n> >\n> > operator+(const std::string &, const std::string &)\n> >\n> > Signed-off-by: Khem Raj \n> > Reviewed-by: Laurent Pinchart \n> >\n> > Use a const char * type instead of auto, and update the commit message\n> > accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 031f96e28449..b6c6ade5ebaf 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)\n> >\n> > /* Creata a device ID from the USB devices vendor and product ID. */\n> > std::string deviceId;\n> > - for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > + for (const char *name : { \"idVendor\", \"idProduct\" }) {\n> > std::ifstream file(path + \"/../\" + name);\n> >\n> > if (!file.is_open())","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 A543EC32EA\n\tfor ;\n\tSun, 28 Mar 2021 16:56:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D4396877F;\n\tSun, 28 Mar 2021 18:56:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DDF66084F\n\tfor ;\n\tSun, 28 Mar 2021 18:56:12 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0093436;\n\tSun, 28 Mar 2021 18:56:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pC9xHm9V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616950572;\n\tbh=gzTtcaDee2HDdQeEhsDueUED8Y8KaNCTMFF1COZ8HOY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pC9xHm9VptfO2nfYh/sVLJc2A/h4oPhHFp2HPPLb8fal3ZFO1H9bjw2/LxzLlz617\n\tsMD+yUteFIxZQ/JopEyn8J+o+gzDHhtJPuD7R9bg8JurRTy6MD15bf/nbYELZ32RU+\n\tmf6TYUJCWgtW8j1sYzKkYOoScLC2vRjXVvo5+4x4=","Date":"Sun, 28 Mar 2021 19:55:28 +0300","From":"Laurent Pinchart ","To":"Khem Raj ","Message-ID":"","References":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15969,"web_url":"https://patchwork.libcamera.org/comment/15969/","msgid":"","date":"2021-03-28T19:47:36","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","submitter":{"id":62,"url":"https://patchwork.libcamera.org/api/people/62/","name":"Khem Raj","email":"raj.khem@gmail.com"},"content":"On Sun, Mar 28, 2021 at 9:56 AM Laurent Pinchart\n wrote:\n>\n> Hi Khem,\n>\n> On Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:\n> > Thanks Laurent, for this, I know it was on my plate to address your\n> > feedback but I could not get to it thus far\n>\n> No worries. I was investigating this issue, to have a better\n> understanding of why the lifetime extension was a problem, and have\n> reworded the commit message as a result, so I thought it was worth\n> posting a v2. I'd appreciate if you could test with gcc-11 when you'll\n> have a chance, but there's no urgency.\n\nYes I tested v2 just now. It works with gcc11 just fine.\n\n>\n> > On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:\n> > >\n> > > From: Khem Raj \n> > >\n> > > A range-based for loop whose range expression is an array of char\n> > > pointers and range variable declaration is a const reference to a\n> > > std::string creates a temporary string from the char pointer and binds\n> > > the range variable reference to it. This creates a const reference to a\n> > > temporary, which is valid in C++, and extends the lifetime of the\n> > > temporary to the lifetime of the reference.\n> > >\n> > > However, lifetime extension in range-based for loops is considered as a\n> > > sign of a potential issue, as a temporary is created for every\n> > > iteration, which can be costly, and the usage of a reference in the\n> > > range declaration doesn't make it obvious that the code isn't simply\n> > > binding a reference to an existing object. gcc 11, with the\n> > > -Wrange-loop-construct option, flags this:\n> > >\n> > > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string > > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]\n> > > | 432 | for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > > | | ^~~~\n> > >\n> > > To please the compiler, make the range variable a const char *. This may\n> > > bring a tiny performance improvement, as the name is only used once, in\n> > > a location where the compiler can use\n> > >\n> > > operator+(const std::string &, const char *)\n> > >\n> > > instead of\n> > >\n> > > operator+(const std::string &, const std::string &)\n> > >\n> > > Signed-off-by: Khem Raj \n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > Use a const char * type instead of auto, and update the commit message\n> > > accordingly.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 031f96e28449..b6c6ade5ebaf 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)\n> > >\n> > > /* Creata a device ID from the USB devices vendor and product ID. */\n> > > std::string deviceId;\n> > > - for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > > + for (const char *name : { \"idVendor\", \"idProduct\" }) {\n> > > std::ifstream file(path + \"/../\" + name);\n> > >\n> > > if (!file.is_open())\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 3439DC32ED\n\tfor ;\n\tSun, 28 Mar 2021 19:48:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 738EF68784;\n\tSun, 28 Mar 2021 21:48:08 +0200 (CEST)","from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 301166084F\n\tfor ;\n\tSun, 28 Mar 2021 21:48:07 +0200 (CEST)","by mail-qv1-xf33.google.com with SMTP id q12so5509867qvc.8\n\tfor ;\n\tSun, 28 Mar 2021 12:48:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"phe/uKkW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6XGAJABsY2f8IWLmpCyNDCur9n+/i8KCpkW60/PShE0=;\n\tb=phe/uKkWGnFe1C/n8zo/0IQjFQIgQ2fHjv4y2Tc0qMKMOYgS/ALv3Au+h74l8vatOU\n\tH9rX5SWH59YKPVmyiklbG7k/hNEgU3AFwYpLvgJd2H7PWMWnn8o50a7Erw7/LCkP1RTO\n\tFcB3WzhSVAyKmLG91MIWqWxmz2zNqM9YHStLrr00URgr9cf9DWcEb+Cyv8fvRKltqiJa\n\t6tuF8u4YzxiPorALHFv6KDbb7By4PFe2yVbyD4uBqTVgyFmiMaDIBsGueVzfo9R03dG4\n\tNCN2TztZsj3FR/NFCJKzLfZq5HKtksSEsgv3r3nEHdfIQkf79fnWIDO2xvXOTh4VlvTt\n\tM6lw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6XGAJABsY2f8IWLmpCyNDCur9n+/i8KCpkW60/PShE0=;\n\tb=lTnYs8+OQTvmYjkTwmpgWwum6u9AUjG42w0ZnarkPW/mea5soyCUC0hAhSSG8W0qdf\n\toE71XEybhUvqjHj16U5S3JpeQi/CFTGMt+j+zY/mE46DoP85RQCBq4hTvnRUxqcQGwAM\n\t5Hp9A5Vr3Y8vVhW/k/hgZpMB+mRtlsj+F8/yBp6r0/Q/Mh8HmbEjGBSNY3IHZGf3np1J\n\toDzONISP9vapAOCLcd4n1xOpqwSTHwWQ6ZU9+phE7dUD3ruRXLYu1b1VCnDY84p6E6kb\n\tD+6XZqjlcSHUYv9KnCoxshW9zb+xD2O9iRnfDN758B7oH60hBU0thmFpHDOH7CJ5l+Au\n\tDuAQ==","X-Gm-Message-State":"AOAM530FCrV9nRoqfFHvFCIWlJYSbvsu2bURFHYzgn+2cTpEOonhmKw1\n\tWWn//pUu7AaeVNsT+V/AMIO+n2ILlh9u+dNIoXAuS13/c0M=","X-Google-Smtp-Source":"ABdhPJzDeUTWDH5JGG5jcPM4AfXO3EzQHAVAMe8UN0Xg4SX8tuPqgX+22IriiEQyIg7LEzGh0YBy6pPsb6twOhrV5I0=","X-Received":"by 2002:a05:6214:f27:: with SMTP id\n\tiw7mr22708377qvb.50.1616960885898; \n\tSun, 28 Mar 2021 12:48:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Khem Raj ","Date":"Sun, 28 Mar 2021 12:47:36 -0700","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15970,"web_url":"https://patchwork.libcamera.org/comment/15970/","msgid":"","date":"2021-03-28T20:08:04","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Khem,\n\nOn Sun, Mar 28, 2021 at 12:47:36PM -0700, Khem Raj wrote:\n> On Sun, Mar 28, 2021 at 9:56 AM Laurent Pinchart wrote:\n> > On Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:\n> > > Thanks Laurent, for this, I know it was on my plate to address your\n> > > feedback but I could not get to it thus far\n> >\n> > No worries. I was investigating this issue, to have a better\n> > understanding of why the lifetime extension was a problem, and have\n> > reworded the commit message as a result, so I thought it was worth\n> > posting a v2. I'd appreciate if you could test with gcc-11 when you'll\n> > have a chance, but there's no urgency.\n> \n> Yes I tested v2 just now. It works with gcc11 just fine.\n\nThank you. I've pushed the patch.\n\nThank you for raising this issue in the first place. As always with C++\nI've spent too much time trying to understand the problem and its\nimplications, but it was quite informative. \n\n> > > On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:\n> > > >\n> > > > From: Khem Raj \n> > > >\n> > > > A range-based for loop whose range expression is an array of char\n> > > > pointers and range variable declaration is a const reference to a\n> > > > std::string creates a temporary string from the char pointer and binds\n> > > > the range variable reference to it. This creates a const reference to a\n> > > > temporary, which is valid in C++, and extends the lifetime of the\n> > > > temporary to the lifetime of the reference.\n> > > >\n> > > > However, lifetime extension in range-based for loops is considered as a\n> > > > sign of a potential issue, as a temporary is created for every\n> > > > iteration, which can be costly, and the usage of a reference in the\n> > > > range declaration doesn't make it obvious that the code isn't simply\n> > > > binding a reference to an existing object. gcc 11, with the\n> > > > -Wrange-loop-construct option, flags this:\n> > > >\n> > > > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string > > > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]\n> > > > | 432 | for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > > > | | ^~~~\n> > > >\n> > > > To please the compiler, make the range variable a const char *. This may\n> > > > bring a tiny performance improvement, as the name is only used once, in\n> > > > a location where the compiler can use\n> > > >\n> > > > operator+(const std::string &, const char *)\n> > > >\n> > > > instead of\n> > > >\n> > > > operator+(const std::string &, const std::string &)\n> > > >\n> > > > Signed-off-by: Khem Raj \n> > > > Reviewed-by: Laurent Pinchart \n> > > >\n> > > > Use a const char * type instead of auto, and update the commit message\n> > > > accordingly.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index 031f96e28449..b6c6ade5ebaf 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)\n> > > >\n> > > > /* Creata a device ID from the USB devices vendor and product ID. */\n> > > > std::string deviceId;\n> > > > - for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> > > > + for (const char *name : { \"idVendor\", \"idProduct\" }) {\n> > > > std::ifstream file(path + \"/../\" + name);\n> > > >\n> > > > if (!file.is_open())","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 6FFE5C32EA\n\tfor ;\n\tSun, 28 Mar 2021 20:08:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFFC86877F;\n\tSun, 28 Mar 2021 22:08:49 +0200 (CEST)","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 7AC436084F\n\tfor ;\n\tSun, 28 Mar 2021 22:08:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C083A323;\n\tSun, 28 Mar 2021 22:08:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mu5hRBqt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616962127;\n\tbh=IRG+SWgh23cFgsEuCZlcBsteBB3MLXY6WbbgNnIlqco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mu5hRBqt+krsmVOaAzDGc2jmoV7AXDtvfZD/nho9kLyEJF7FFRZjtBtHbPhyVt4VL\n\toYdk89rHglwQeZCptM5otNcsamSoApnBJhDMKuDw/Tvo2ulS6P7IoMo0K1NNM9cROz\n\tZfP0li6AQXa0RiLJwvKz6xQnofJuSvjmktdvBYIM=","Date":"Sun, 28 Mar 2021 23:08:04 +0300","From":"Laurent Pinchart ","To":"Khem Raj ","Message-ID":"","References":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16003,"web_url":"https://patchwork.libcamera.org/comment/16003/","msgid":"<20210329080040.ffp3jmpj3d4v2bty@uno.localdomain>","date":"2021-03-29T08:00:40","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello\n\nOn Sun, Mar 28, 2021 at 05:57:41AM +0300, Laurent Pinchart wrote:\n> From: Khem Raj \n>\n> A range-based for loop whose range expression is an array of char\n> pointers and range variable declaration is a const reference to a\n> std::string creates a temporary string from the char pointer and binds\n> the range variable reference to it. This creates a const reference to a\n> temporary, which is valid in C++, and extends the lifetime of the\n> temporary to the lifetime of the reference.\n>\n> However, lifetime extension in range-based for loops is considered as a\n> sign of a potential issue, as a temporary is created for every\n> iteration, which can be costly, and the usage of a reference in the\n> range declaration doesn't make it obvious that the code isn't simply\n> binding a reference to an existing object. gcc 11, with the\n> -Wrange-loop-construct option, flags this:\n>\n> uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]\n> | 432 | for (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> | | ^~~~\n>\n\nAmusing!\n\n> To please the compiler, make the range variable a const char *. This may\n> bring a tiny performance improvement, as the name is only used once, in\n> a location where the compiler can use\n>\n> \toperator+(const std::string &, const char *)\n>\n> instead of\n>\n> \toperator+(const std::string &, const std::string &)\n>\n> Signed-off-by: Khem Raj \n> Reviewed-by: Laurent Pinchart \n>\n> Use a const char * type instead of auto, and update the commit message\n> accordingly.\n>\n> Signed-off-by: Laurent Pinchart \n\nNice catch!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 031f96e28449..b6c6ade5ebaf 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)\n>\n> \t/* Creata a device ID from the USB devices vendor and product ID. */\n> \tstd::string deviceId;\n> -\tfor (const std::string &name : { \"idVendor\", \"idProduct\" }) {\n> +\tfor (const char *name : { \"idVendor\", \"idProduct\" }) {\n> \t\tstd::ifstream file(path + \"/../\" + name);\n>\n> \t\tif (!file.is_open())\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 1CCAAC32EF\n\tfor ;\n\tMon, 29 Mar 2021 08:00:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94C586877E;\n\tMon, 29 Mar 2021 10:00:07 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 156F66877E\n\tfor ;\n\tMon, 29 Mar 2021 10:00:06 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 8DEB3C0019;\n\tMon, 29 Mar 2021 08:00:05 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 10:00:40 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210329080040.ffp3jmpj3d4v2bty@uno.localdomain>","References":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210328025741.30246-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo:\n\tAvoid reference to temporary object","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]