[libcamera-devel] [PATCH 4/4] ipa: raspberrypi: fix use of uninitialized fields

[Kieran Bingham]

Hi Laurent,

On 07/10/2020 13:54, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wed, Oct 07, 2020 at 03:52:21PM +0300, Tomi Valkeinen wrote:
>> On 07/10/2020 15:47, Kieran Bingham wrote:
>>> On 07/10/2020 12:07, Tomi Valkeinen wrote:
>>>> These fields are not initialized, but are used. Set them to 0.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
>>>> ---
>>>>  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
>>>> index ba7ae09..23374d5 100644
>>>> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
>>>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
>>>> @@ -116,9 +116,9 @@ private:
>>>>  	std::string exposure_mode_name_;
>>>>  	std::string constraint_mode_name_;
>>>>  	double ev_;
>>>> -	double flicker_period_;
>>>> -	double fixed_shutter_;
>>>> -	double fixed_analogue_gain_;
>>>> +	double flicker_period_ = 0;
>>>
>>>  = 0.0; ?
>>>
>>> Also - this is setting the initialisation in the header definition,
>>> rather than the implementation where we would normally do the
>>> initialisation. Any reason for that?
>>
>> Any reason not to initialize in the header? I think it's much nicer to
>> initialize there when you are setting to a simple literal. It's very
>> easy to miss the init in the implementation file, especially if you
>> have multiple constructors.
> 
> That's the current coding style. I didn't even know this was possible
> :-) I'm not opposed to reconsidering this, but it should then be changed
> globally.

I think for me, equally - this was a "didn't realise we could", and
"we've always done it that way" ;-) - but I do feel like it's better.

I bet we can reduce a few constructor initialiser lists with this too!

And handling multiple constructors in one hit (I'm not sure how many of
our classes have multiple constructors, but there's a few I'm sure)
seems like an instant win too.

/me wishes C++ would just initialise all PODs to 0 ;-)

-- 
Regards
--
Kieran