[libcamera-devel] [PATCH] uvcvideo: Use auto variable to avoid range loop warnings

[Laurent Pinchart]

Hi Khem,

Thank you for the patch.

On Wed, Mar 03, 2021 at 03:21:26PM -0800, Khem Raj wrote:
> With c++17 loop range bases are defined where copy is obvious since
> iterator returns a copy and not reference, gcc11 will emit a warning
> about 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" }) {
> |       |                                 ^~~~

Looks like I should add gcc 11 to my build tests :-)

> Therefore making it explicit is better
> 
> Signed-off-by: Khem Raj <raj.khem at gmail.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 031f96e2..ef23ece7 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 auto name : { "idVendor", "idProduct" }) {

What would you think about making this

	for (const char *name : { "idVendor", "idProduct" }) {

? We tend to only use auto when the explicit type is either impossible
to type, or would be too long, as an explicit type makes it easier for
the reader to know the type of the variable. I however have to say that
using auto here since the beginning would have prevented this bug from
happening in the first place. I'll let you decide what you think is
best, and if you opt for const char *name, I can change this when
applying, there's no need to resend the patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  		std::ifstream file(path + "/../" + name);
>  
>  		if (!file.is_open())

-- 
Regards,

Laurent Pinchart