[libcamera-devel] [PATCH v2] libcamera: pipeline: uvcvideo: Avoid reference to temporary object

[Jacopo Mondi]

Hello

On Sun, Mar 28, 2021 at 05:57:41AM +0300, Laurent Pinchart wrote:
> From: Khem Raj <raj.khem at gmail.com>
>
> A range-based for loop whose range expression is an array of char
> pointers and range variable declaration is a const reference to a
> std::string creates a temporary string from the char pointer and binds
> the range variable reference to it. This creates a const reference to a
> temporary, which is valid in C++, and extends the lifetime of the
> temporary to the lifetime of the reference.
>
> However, lifetime extension in range-based for loops is considered as a
> sign of a potential issue, as a temporary is created for every
> iteration, which can be costly, and the usage of a reference in the
> range declaration doesn't make it obvious that the code isn't simply
> binding a reference to an existing object. gcc 11, with the
> -Wrange-loop-construct option, flags this:
>
> uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha
> r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]
> |   432 |         for (const std::string &name : { "idVendor", "idProduct" }) {
> |       |                                 ^~~~
>

Amusing!

> To please the compiler, make the range variable a const char *. This may
> bring a tiny performance improvement, as the name is only used once, in
> a location where the compiler can use
>
> 	operator+(const std::string &, const char *)
>
> instead of
>
> 	operator+(const std::string &, const std::string &)
>
> Signed-off-by: Khem Raj <raj.khem at gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Use a const char * type instead of auto, and update the commit message
> accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Nice catch!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 031f96e28449..b6c6ade5ebaf 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)
>
>  	/* Creata a device ID from the USB devices vendor and product ID. */
>  	std::string deviceId;
> -	for (const std::string &name : { "idVendor", "idProduct" }) {
> +	for (const char *name : { "idVendor", "idProduct" }) {
>  		std::ifstream file(path + "/../" + name);
>
>  		if (!file.is_open())
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel