[libcamera-devel] [PATCH] cam: options: Fix unitialized variable warning

[Kieran Bingham]

Hi Laurent,

On 23/11/2019 10:16, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Sat, Nov 23, 2019 at 10:36:51AM +0200, Laurent Pinchart wrote:
>> gcc 8 and 9 complain about the OptionValue::integer_ member being
>> possibly used initialized when compiled in release mode. I haven't been
>> able to find where this could be the case, and the compiler error
>> message isn't helpful:
>>
>> In file included from ../../src/cam/options.cpp:14:
>> ../../src/cam/options.h: In member function ‘bool OptionsBase<T>::parseValue(const T&, const Option&, const char*) [with T = std::__cxx11::basic_string<char>]’:
>> ../../src/cam/options.h:84:7: error: ‘<anonymous>.OptionValue::integer_’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>  class OptionValue
>>        ^~~~~~~~~~~
>> ../../src/cam/options.h: In member function ‘bool OptionsBase<T>::parseValue(const T&, const Option&, const char*) [with T = int]’:
>> ../../src/cam/options.h:84:7: error: ‘<anonymous>.OptionValue::integer_’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>  class OptionValue
>>        ^~~~~~~~~~~
>>
>> Furthermore valgrind doesn't report any issue. This is likely a false
>> positive, but fix it nonetheless as the fix is cheap.
> 
> Possibly, but let's silence it anyway. It won't hurt for sure.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

OptionValue is indeed leaving the integer_ uninitialised in it's non-int
constructors, though I can't see that it is being used without calling
through the (int) constructor which /would/ initialise it.

Unless perhaps it's because of one of the templated base class usages?

> Template class OptionsBase<int>;

> class OptionsParser
> {
> public:
>         class Options : public OptionsBase<int>
> 

Anyway, initialising it seems the 'correct' thing to do, though I really
can't see that it could be used.

But it's likely hard for the compiler to 'prove' that.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 
> Thanks
>    j
> 
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>>  src/cam/options.cpp | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
>> index 7e2dfa636ccf..186e853c8294 100644
>> --- a/src/cam/options.cpp
>> +++ b/src/cam/options.cpp

Should this be initialised on the default constructor too for completeness?

OptionValue::OptionValue()
        : type_(ValueNone)
{
}

>> @@ -260,17 +260,17 @@ OptionValue::OptionValue(int value)
>>  }
>>
>>  OptionValue::OptionValue(const char *value)
>> -	: type_(ValueString), string_(value)
>> +	: type_(ValueString), integer_(0), string_(value)
>>  {
>>  }
>>
>>  OptionValue::OptionValue(const std::string &value)
>> -	: type_(ValueString), string_(value)
>> +	: type_(ValueString), integer_(0), string_(value)
>>  {
>>  }
>>
>>  OptionValue::OptionValue(const KeyValueParser::Options &value)
>> -	: type_(ValueKeyValue), keyValues_(value)
>> +	: type_(ValueKeyValue), integer_(0), keyValues_(value)
>>  {
>>  }
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran