| Message ID | 20201007110743.55561-5-tomi.valkeinen@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, 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@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? > + double fixed_shutter_ = 0; > + double fixed_analogue_gain_ = 0; These get initialised in Agc::Agc(Controller *controller) ... Oh - wait, I see you've removed that in patch 2/4! So technically this is a defect introduced by patch 2/4 - so perhaps it should be squashed there in some form or another. > }; > > } // namespace RPiController >
On 07/10/2020 15:47, Kieran Bingham wrote: > Hi Tomi, > > 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@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. Tomi
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@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.
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@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 ;-)
Hi Kieran, On Wed, Oct 07, 2020 at 02:00:49PM +0100, Kieran Bingham wrote: > On 07/10/2020 13:54, Laurent Pinchart wrote: > > 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@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. There are drawbacks too, it splits initialization between two files, making it easier to miss things. Let's consider the pros and cons and then make a decision. > /me wishes C++ would just initialise all PODs to 0 ;-)
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; + double fixed_shutter_ = 0; + double fixed_analogue_gain_ = 0; }; } // namespace RPiController
These fields are not initialized, but are used. Set them to 0. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> --- src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)