[libcamera-devel] [PATCH] qcam: viewfinder_gl: selectColorSpace: fix maybe uninitialized warning

[Marco Felsch]

Hi Laurent,

On 22-09-03, Laurent Pinchart wrote:
> On Sat, Sep 03, 2022 at 02:32:40AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Sat, Sep 03, 2022 at 02:31:39AM +0300, Laurent Pinchart wrote:
> > > Hi Marco,
> > > 
> > > Going back to the patch now.
> > > 
> > > On Fri, Sep 02, 2022 at 04:11:36PM +0200, Marco Felsch via libcamera-devel wrote:
> > > > Fix gcc11 warning introduced in commit 251f0534 ("qcam: viewfinder_gl:
> > > > Take color space into account for YUV rendering"). Fix it by making the
> > > > full range as default.
> > > 
> > > I'd like to mention the result of the investigation I detailed in a
> > > separate reply in this mail thread. Would you be fine with the following
> > > commit message ?
> > > 
> > > Commit 251f0534 ("qcam: viewfinder_gl: Take color space into account for
> > > YUV rendering") introduced a maybe-uninitialized warning with gcc 11 and
> > > 12. The compiler is correct, as C++17 allows initializing a scoped enum
> > > from an integer using direct-list-initialization, even if the integer
> > > value doesn't match any of the enumerators for the scoped enum ([1]).
> > > 
> > > Fix the warning by making the full range the default.
> > > 
> > > https://en.cppreference.com/w/cpp/language/enum#enum_relaxed_init_cpp17
> > > 
> > > > Fixes: 251f0534 ("qcam: viewfinder_gl: Take color space into account for YUV rendering")
> > 
> > I also forgot to mention, Fixes tags should use 12 digits for the commit
> > ID. I can also address this locally.
> 
> Also, gcc 12.2.0 still warns about the second switch statement in the
> same function. I'll submit a v2 of your patch to fix that as well.

Of course, thanks for the investigation on this topic.

Regards,
  Marco

> > > > Signed-off-by: Marco Felsch <m.felsch at pengutronix.de>
> > > > ---
> > > >  src/qcam/viewfinder_gl.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> > > > index e2aa2470..501c5bae 100644
> > > > --- a/src/qcam/viewfinder_gl.cpp
> > > > +++ b/src/qcam/viewfinder_gl.cpp
> > > > @@ -367,6 +367,7 @@ void ViewFinderGL::selectColorSpace(const libcamera::ColorSpace &colorSpace)
> > > >  	double offset;
> > > >  
> > > >  	switch (colorSpace.range) {
> > > > +	default: /* Avoid gcc11 maybe-uninitialized warning */
> > > 
> > > Given that this is a valid behaviour for the compiler, and not a false
> > > positive as I initially thought, I think we can drop the comment. If you
> > > don't mind I'll also move the default one line down, just right after
> > > the Full case.
> > > 
> > > There's no need to submit a v2 if you're fine with these changes, I'll
> > > apply them locally.
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > 
> > > >  	case libcamera::ColorSpace::Range::Full:
> > > >  		offset = 0.0;
> > > >  		break;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>