[{"id":3453,"web_url":"https://patchwork.libcamera.org/comment/3453/","msgid":"<20200115193554.GB977577@oden.dyn.berto.se>","date":"2020-01-15T19:35:54","subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2020-01-14 22:14:52 +0200, Laurent Pinchart wrote:\n> When emplacing an element in a std::map, std::piecewise_construct is\n> required to forward the parameters to the constructors of the key and\n> mapped type, respectively. However, when the caller already has an\n> instance of the mapped type, forwarding it to the mapped type copy\n> constructor isn't really required, as the copy constructor would be\n> called anyway. Drop std::piecewise_construct in those cases. This does\n> not incur any performance cost.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/uvcvideo.cpp | 4 +---\n> src/libcamera/pipeline/vimc.cpp | 4 +---\n> 2 files changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 83093676ec73..29afb121aa46 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -365,9 +365,7 @@ int UVCCameraData::init(MediaEntity *entity)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tctrls.emplace(std::piecewise_construct,\n> -\t\t\t std::forward_as_tuple(id),\n> -\t\t\t std::forward_as_tuple(range));\n> +\t\tctrls.emplace(id, range);\n> \t}\n> \n> \tcontrolInfo_ = std::move(ctrls);\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index c99560a45cfa..b1054d307ea2 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -448,9 +448,7 @@ int VimcCameraData::init(MediaDevice *media)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tctrls.emplace(std::piecewise_construct,\n> -\t\t\t std::forward_as_tuple(id),\n> -\t\t\t std::forward_as_tuple(range));\n> +\t\tctrls.emplace(id, range);\n> \t}\n> \n> \tcontrolInfo_ = std::move(ctrls);\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":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A8586045C\n\tfor ;\n\tWed, 15 Jan 2020 20:35:56 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id n12so13650865lfe.3\n\tfor ;\n\tWed, 15 Jan 2020 11:35:56 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tz7sm11093304lfa.81.2020.01.15.11.35.54\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 15 Jan 2020 11:35:54 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TGORK9GFSJWDS9GIJB+XB7mU2dtTmCAGbGcwh8bxAjI=;\n\tb=SYSCNRycTLxnKM2ckVGEAA4NlNlTxePcb7QUp7XbDIFH9KBM6s0ygT++tDqlcdytgo\n\tatF8+Xu4DS1goxxzPaZ2IF3wnVaQMy6vak8IvoSPIryaGog/MEUZfhFIomQ7+s9tWeYi\n\trnaruqt6SN9jZsjvbpfWhNVkL67Kd0mNNbnjSmnZPkvQCv/OeB2e+7xXqZdocZU44hGj\n\t05aAjq3SAbBo4dobx70rxbasP943J0LWrCFeSwud4mFKS+mMixQK+XcEik0adxMttr0D\n\tI7Y+a8LmVYFRrySD0NB2LFiAR0IqhGUMl5Nb7zUeqpNacwN32FHXnxPWCIa29WlVw2YF\n\tnqVA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TGORK9GFSJWDS9GIJB+XB7mU2dtTmCAGbGcwh8bxAjI=;\n\tb=goMWGIjN17uO+0vbpkTfTZOJWyxOZhE3to6uan6Wv/crL8bp4/i8awQovm9pUjHHA7\n\tY8gLqc8ybywa7HAdqI74AkNpEgoIshWEPdxfg2fVEfXdoyMHdq/q7uUW/MAJCa8fJtKZ\n\ta9G5wgQFSEbcKD2JbHY26PMwXjVueyOvca7FqRi92hTaG8t3T29k/7aiVfdRapXa2B0p\n\tJjr/29jnB0AgLq4QeYuTM4nWCAtqw2/9vRhTWILR6iL/RG3BG/Q+M76WBZsEwwd8iarE\n\tNBP6lscJWkG6mfWXc2BMCJE4byso9WCWj/opWF99Cu/txnaKZ95fTjfgIQsIqxA4XBPs\n\t4NOQ==","X-Gm-Message-State":"APjAAAWhxtftwKE/ixWeoaHmojTJkfS+K/PmLbeZH+HDUqfAp/Ij5Ltx\n\tg+YIXQEdGyohx8Vm9uEYrYmGtg==","X-Google-Smtp-Source":"APXvYqxSl1uifHbU4yOvKmKZaqSxrFZGuMsHxGqE0f6e285KBQxyLJQraMoVm2EILAwU7LybTDladg==","X-Received":"by 2002:ac2:531b:: with SMTP id c27mr275466lfh.91.1579116955602; \n\tWed, 15 Jan 2020 11:35:55 -0800 (PST)","Date":"Wed, 15 Jan 2020 20:35:54 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200115193554.GB977577@oden.dyn.berto.se>","References":"<20200114201452.14078-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200114201452.14078-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","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","X-List-Received-Date":"Wed, 15 Jan 2020 19:35:56 -0000"}},{"id":3459,"web_url":"https://patchwork.libcamera.org/comment/3459/","msgid":"<286c7745-833a-17d2-636f-feef0e2f6788@ideasonboard.com>","date":"2020-01-16T12:41:05","subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nI find the commit message quite terse here, and it doesn't help me in\nunderstanding the meaning behind why the piecewise construct was (or\nshould) be used, or why it's being removed.\n\nPerhaps that's not quite the purpose of the commit message of course\n(it's not supposed to teach us C++), but I feel like this is one point\nwhere a good -easy-to-understand message would be useful. Particularly\nif it helps us prevent this mistake in the future.\n\nNow that we've gone through the purpose of the piecewise_constructor,\nI've tried to rewrite your commit message below to try to clarify my\nunderstanding.\n\nIf you find any of the content more pleasing, or easier to read, feel\nfree to take it in any form.\n\n\nOn 14/01/2020 20:14, Laurent Pinchart wrote:\n> When emplacing an element in a std::map, std::piecewise_construct is\n> required to forward the parameters to the constructors of the key and\n> mapped type, respectively. However, when the caller already has an\n> instance of the mapped type, forwarding it to the mapped type copy\n> constructor isn't really required, as the copy constructor would be\n> called anyway. Drop std::piecewise_construct in those cases. This does\n> not incur any performance cost.\n> \n\n\nWhen emplacing a control on to the ControlInfoMap with an ID and a\nRange, each of the parameters are objects and are directly attributed to\nthe construction of the key value pairs of the ControlInfoMap.\n\nThe use of std::piecewise_construct permits, for example the range to be\nspecified using a non-default constructor (such as {0, 100}) directly in\nthe emplace method, where the parameter values would be forwarded to the\nconstructor.\n\nIn the UVC Video and the VIMC pipeline handlers, this has been overused\nas the parameters to the emplace method are objects themselves, and the\nemplace can use the copy constructors to create the new ControlInfoMap\nentry.\n\nSimplify the code and improve readability by utilising the default\nemplace method for the map which takes a single argument for each of the\ntwo elements.\n\nNo change is expected in the output code generated by this update.\n\n\n> Signed-off-by: Laurent Pinchart \n\nWith or without any update to the commit message, this change is fine.\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/libcamera/pipeline/uvcvideo.cpp | 4 +---\n> src/libcamera/pipeline/vimc.cpp | 4 +---\n> 2 files changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 83093676ec73..29afb121aa46 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -365,9 +365,7 @@ int UVCCameraData::init(MediaEntity *entity)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tctrls.emplace(std::piecewise_construct,\n> -\t\t\t std::forward_as_tuple(id),\n> -\t\t\t std::forward_as_tuple(range));\n> +\t\tctrls.emplace(id, range);\n> \t}\n> \n> \tcontrolInfo_ = std::move(ctrls);\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index c99560a45cfa..b1054d307ea2 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -448,9 +448,7 @@ int VimcCameraData::init(MediaDevice *media)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tctrls.emplace(std::piecewise_construct,\n> -\t\t\t std::forward_as_tuple(id),\n> -\t\t\t std::forward_as_tuple(range));\n> +\t\tctrls.emplace(id, range);\n> \t}\n> \n> \tcontrolInfo_ = std::move(ctrls);\n>","headers":{"Return-Path":"","Received":["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 8948E60457\n\tfor ;\n\tThu, 16 Jan 2020 13:41:08 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAF6F2D2;\n\tThu, 16 Jan 2020 13:41:07 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579178468;\n\tbh=Ke6ajTeGIIPZMveNl5PfOVU4nQwX86C85rNq3z+P7ZY=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=PoLSsvFuom6lUkeZx7+6b1+FTCWq9Mp/bdNF5rBuS1GjdYHAdgI8cDQVbsWzhptWI\n\tX0FZxbfOWb7yIL3plR354C1WqlxDsziOanQR70ccsFq2Ho7u4ucIsU9ahC05RI/Lmb\n\tYyB25ZwF3/mnuiXGSuXs76V7W/EYoRGhAmwEJthk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200114201452.14078-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<286c7745-833a-17d2-636f-feef0e2f6788@ideasonboard.com>","Date":"Thu, 16 Jan 2020 12:41:05 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200114201452.14078-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","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","X-List-Received-Date":"Thu, 16 Jan 2020 12:41:08 -0000"}},{"id":3460,"web_url":"https://patchwork.libcamera.org/comment/3460/","msgid":"<20200116165913.GJ7139@pendragon.ideasonboard.com>","date":"2020-01-16T16:59:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jan 16, 2020 at 12:41:05PM +0000, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> I find the commit message quite terse here, and it doesn't help me in\n> understanding the meaning behind why the piecewise construct was (or\n> should) be used, or why it's being removed.\n> \n> Perhaps that's not quite the purpose of the commit message of course\n> (it's not supposed to teach us C++), but I feel like this is one point\n> where a good -easy-to-understand message would be useful. Particularly\n> if it helps us prevent this mistake in the future.\n\nI could have written a 3-pages message to explain rvalue references,\nperfect forwarding, piewcewise construct and emplace, but I indeed\nthought it wasn't the purpose of the commit message :-)\n\n> Now that we've gone through the purpose of the piecewise_constructor,\n> I've tried to rewrite your commit message below to try to clarify my\n> understanding.\n\nThank you. If not useful for me, I think it can be useful for others\n(which was the point of you rewriting this as I understand it).\n\n> If you find any of the content more pleasing, or easier to read, feel\n> free to take it in any form.\n> \n> On 14/01/2020 20:14, Laurent Pinchart wrote:\n> > When emplacing an element in a std::map, std::piecewise_construct is\n> > required to forward the parameters to the constructors of the key and\n> > mapped type, respectively. However, when the caller already has an\n> > instance of the mapped type, forwarding it to the mapped type copy\n> > constructor isn't really required, as the copy constructor would be\n> > called anyway. Drop std::piecewise_construct in those cases. This does\n> > not incur any performance cost.\n> \n> When emplacing a control on to the ControlInfoMap with an ID and a\n> Range, each of the parameters are objects and are directly attributed to\n> the construction of the key value pairs of the ControlInfoMap.\n> \n> The use of std::piecewise_construct permits, for example the range to be\n> specified using a non-default constructor (such as {0, 100}) directly in\n> the emplace method, where the parameter values would be forwarded to the\n> constructor.\n> \n> In the UVC Video and the VIMC pipeline handlers, this has been overused\n> as the parameters to the emplace method are objects themselves, and the\n> emplace can use the copy constructors to create the new ControlInfoMap\n> entry.\n> \n> Simplify the code and improve readability by utilising the default\n> emplace method for the map which takes a single argument for each of the\n> two elements.\n\nI understand what you're saying, but I'm honestly not sure it's clearer\nI'm afraid. I think we're both confusing, but in different ways :-)\n\nHow about this combined version ?\n\nWhen inserting an element with emplace(), the element is constructed\nin-place with the parameters to the emplace() method being forwarded to\nthe constructor of the element. For std::map containers, the element is\nan std::pair. The constructors of std::pair fall\ninto three categories:\n\n(1) Default, copy and move constructors (and related versions)\n(2) Constructors that take lvalue or rvalue references to T1 and T2\n(3) A forwarding constructor that forwards parameters to the\n constructors of T1 and T2, called \n\nThe first category isn't useful in most cases for std::map::emplace(),\nas the caller usually doesn't have an existing std::pair\nfor the element to be inserted.\n\nThe constructor from the third category is useful to avoid constructing\nintermediate Key or T instances when the caller doesn't have them\navailable. This constructor takes two std::tuple arguments that contain\nthe arguments for the Key and T constructors, respectively. Due to\ntemplate deduction rules, usage of such a constructor couldn't be\ndeduced by the compiler automatically in all cases, so the constructor\ntakes a first argument of type std::piecewise_construct_t that lets the\ncaller force the usage ot the forwarding constructor (also known for\nthis reason as the piecewise constructor). The caller uses a construct\nsuch as\n\n\tmap.emplace(std::piecewise_construct,\n\t\t std::forward_as_tuple(args_for_Key, ...),\n\t\t std::forward_as_tuple(args_for_T, ...));\n\nThis syntax is a bit heavy, but is required to construct Key and T\nin-place from arguments to their non-default constructor (it is also the\nonly std::pair non-default constructor that can be used for non-copyable\nnon-movable types).\n\nWhen the caller of std::map::emplace() already has references to a Key\nand a T, they can be passed to the std::pair piecewise constructor, and\nthis will create std::tuple instance to wrap the Key and T references\narguments to ultimately pass them to the Key and T copy constructors.\n\n\tmap.emplace(std::piecewise_construct,\n\t\t std::forward_as_tuple(Key_value),\n\t\t std::forward_as_tuple(T_value));\n\nWhile this mechanism works, it's unnecessary complex. A constructor of\nstd::pair that takes references to Key and T can be used without any\nperformance penalty, as it will also call the copy constructor of Key\nand T. In this case we can use a simpler constructor of std::pair, and\nthus a simpler call of std::map::emplace.\n\n\tmap.emplace(Key_value, T_value);\n\nWe have a couple occurrences of this above misuse of piecewise\nconstruction. Simplify them, which simplifies the code and reduces the\ngenerated code size.\n\n> No change is expected in the output code generated by this update.\n\nThis is actually not true. We're calling different constructors of\nstd::pair<>, so different code paths are expected. I've tested this, and\nthe code size has actually gone down :-)\n\n> > Signed-off-by: Laurent Pinchart \n> \n> With or without any update to the commit message, this change is fine.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > ---\n> > src/libcamera/pipeline/uvcvideo.cpp | 4 +---\n> > src/libcamera/pipeline/vimc.cpp | 4 +---\n> > 2 files changed, 2 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 83093676ec73..29afb121aa46 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -365,9 +365,7 @@ int UVCCameraData::init(MediaEntity *entity)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > -\t\tctrls.emplace(std::piecewise_construct,\n> > -\t\t\t std::forward_as_tuple(id),\n> > -\t\t\t std::forward_as_tuple(range));\n> > +\t\tctrls.emplace(id, range);\n> > \t}\n> > \n> > \tcontrolInfo_ = std::move(ctrls);\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index c99560a45cfa..b1054d307ea2 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -448,9 +448,7 @@ int VimcCameraData::init(MediaDevice *media)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > -\t\tctrls.emplace(std::piecewise_construct,\n> > -\t\t\t std::forward_as_tuple(id),\n> > -\t\t\t std::forward_as_tuple(range));\n> > +\t\tctrls.emplace(id, range);\n> > \t}\n> > \n> > \tcontrolInfo_ = std::move(ctrls);","headers":{"Return-Path":"","Received":["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 DB03360457\n\tfor ;\n\tThu, 16 Jan 2020 17:59:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 548252D2;\n\tThu, 16 Jan 2020 17:59:27 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579193967;\n\tbh=1xabbIhVecdDO8xt1U2gTiSYSTmdqEWz6zpNWb1JZdM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iYT1TZm+e0SMMExCqtGX7UXKtaBmxtM0gxogfunOFVmM9BysN6bVqcFliyNaBr5Mo\n\tbwWnwzEKoLAEYsLb50JuDNMtoGbL1lm1qBdI4GdRjTM50T0o7WKpxa3auzXm+Kz/YQ\n\tUriIwvWnZsWBvrOdWuYyQi8NQq9tV7vNli3LWGMo=","Date":"Thu, 16 Jan 2020 18:59:13 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200116165913.GJ7139@pendragon.ideasonboard.com>","References":"<20200114201452.14078-1-laurent.pinchart@ideasonboard.com>\n\t<286c7745-833a-17d2-636f-feef0e2f6788@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<286c7745-833a-17d2-636f-feef0e2f6788@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Remove\n\tstd::piecewise_construct where not necessary","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","X-List-Received-Date":"Thu, 16 Jan 2020 16:59:28 -0000"}}]