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

11738 diff mbox series

From: Khem Raj <raj.khem@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" }) {
|       |                                 ^~~~

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@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Use a const char * type instead of auto, and update the commit message
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Thanks Laurent, for this, I know it was on my plate to address your
feedback but I could not get to it thus far

On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Khem Raj <raj.khem@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" }) {
> |       |                                 ^~~~
>
> 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@gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Use a const char * type instead of auto, and update the commit message
> accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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
>

Hi Khem,

On Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:
> Thanks Laurent, for this, I know it was on my plate to address your
> feedback but I could not get to it thus far

No worries. I was investigating this issue, to have a better
understanding of why the lifetime extension was a problem, and have
reworded the commit message as a result, so I thought it was worth
posting a v2. I'd appreciate if you could test with gcc-11 when you'll
have a chance, but there's no urgency.

> On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:
> >
> > From: Khem Raj <raj.khem@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" }) {
> > |       |                                 ^~~~
> >
> > 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@gmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Use a const char * type instead of auto, and update the commit message
> > accordingly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  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())

On Sun, Mar 28, 2021 at 9:56 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Khem,
>
> On Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:
> > Thanks Laurent, for this, I know it was on my plate to address your
> > feedback but I could not get to it thus far
>
> No worries. I was investigating this issue, to have a better
> understanding of why the lifetime extension was a problem, and have
> reworded the commit message as a result, so I thought it was worth
> posting a v2. I'd appreciate if you could test with gcc-11 when you'll
> have a chance, but there's no urgency.

Yes I tested v2 just now. It works with gcc11 just fine.

>
> > On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:
> > >
> > > From: Khem Raj <raj.khem@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" }) {
> > > |       |                                 ^~~~
> > >
> > > 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@gmail.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > Use a const char * type instead of auto, and update the commit message
> > > accordingly.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  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

Hi Khem,

On Sun, Mar 28, 2021 at 12:47:36PM -0700, Khem Raj wrote:
> On Sun, Mar 28, 2021 at 9:56 AM Laurent Pinchart wrote:
> > On Sat, Mar 27, 2021 at 10:42:07PM -0700, Khem Raj wrote:
> > > Thanks Laurent, for this, I know it was on my plate to address your
> > > feedback but I could not get to it thus far
> >
> > No worries. I was investigating this issue, to have a better
> > understanding of why the lifetime extension was a problem, and have
> > reworded the commit message as a result, so I thought it was worth
> > posting a v2. I'd appreciate if you could test with gcc-11 when you'll
> > have a chance, but there's no urgency.
> 
> Yes I tested v2 just now. It works with gcc11 just fine.

Thank you. I've pushed the patch.

Thank you for raising this issue in the first place. As always with C++
I've spent too much time trying to understand the problem and its
implications, but it was quite informative. 

> > > On Sat, Mar 27, 2021 at 7:58 PM Laurent Pinchart wrote:
> > > >
> > > > From: Khem Raj <raj.khem@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" }) {
> > > > |       |                                 ^~~~
> > > >
> > > > 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@gmail.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > Use a const char * type instead of auto, and update the commit message
> > > > accordingly.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  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())

Hello

On Sun, Mar 28, 2021 at 05:57:41AM +0300, Laurent Pinchart wrote:
> From: Khem Raj <raj.khem@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@gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Use a const char * type instead of auto, and update the commit message
> accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Nice catch!
Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel