[PATCH v2] android: camera_capabilities: Fix GCC 14 warning

[Laurent Pinchart]

Hi Barnabás,

On Mon, May 27, 2024 at 06:32:04PM +0000, Barnabás Pőcze wrote:
> 2024. május 27., hétfő 19:06 keltezéssel, Laurent Pinchart írta:
> > On Mon, May 27, 2024 at 07:24:16PM +0300, Laurent Pinchart wrote:
> > > On Mon, May 27, 2024 at 02:34:19PM +0100, Kieran Bingham wrote:
> > > > Quoting Barnabás Pőcze (2024-05-27 14:24:29)
> > > > > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary":
> > > > >
> > > > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’:
> > > > > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
> > > > >  1084 |                 const Span<const Rectangle>& rects =
> > > > >       |                                              ^~~~~
> > > > > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’
> > > > >  1085 |                         properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> > > > >       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > The return value of `value_or()` is indeed a temporary, but
> > > > > binding it to a reference extends its lifetime. Avoid the
> > > > > warning by not using a reference; this does not make much
> > > > > difference since `value_or()` does not return a reference.
> > 
> > Actually, I'd like to update the commit message. The issue isn't so much
> > that value_or() returns a temporary, but that the parameter passed to
> > value_or() is destroyed at the end of the statement. Due to copy elision
> > (I think), value_or() may return the instance it receives as a
> > parameter.
> 
> I am not entirely sure about that. Note https://en.cppreference.com/w/cpp/utility/optional/value_or
> specifically:
> 
>   2) Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)).
> 
> In my reading when returning the default value, a new value of type T
> is constructed from the argument (which is a forwarding reference).
> 
> So in this specific case, I would say that we're constructing a `Span<const Rectangle>`
> from a `Span<const Rectangle>&&` (the argument), and returning that, so it
> is a new separate object from the argument.
> 
> I am fairly certain GCC is in the wrong here, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115229
> I opened that issue after running into specific warning. Note that if you remove
> the `int *p` part  from the code in the issue then the warning disappears.

I *think* you're right, but I can't make up my mind for sure here. I'm
not even totally sure about which temporary gcc think the dangling
reference relates to :-S

I'll use your original commit message.

> > Barnabás, Would the following commit message work for you (and do you
> > think it's accurate) ?
> > 
> > --------
> > The return value of `value_or()` is a temporary, and binding it to a
> > reference extends its lifetime. However, the parameter passed to the
> > `value_or()` function is destroyed at the end of the statement. With
> > copy elision, the function may effectively return the instance that it
> > receives as a parameter, leading to a dangling reference to a temporary.
> > 
> > Fix this by storing a copy of the value. As Span is a lightweight class,
> > the overhead is negligible.
> > --------
> > 
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > I'm working on adding gcc 14 tests in our CI.
> > >
> > > > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > > > ---
> > > > >  src/android/camera_capabilities.cpp | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > > > index 6f4d48de..71043e12 100644
> > > > > --- a/src/android/camera_capabilities.cpp
> > > > > +++ b/src/android/camera_capabilities.cpp
> > > > > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > >         }
> > > > >
> > > > >         {
> > > > > -               const Span<const Rectangle> &rects =
> > > > > +               const Span<const Rectangle> rects =
> > > > >                         properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> > > > >                 std::vector<int32_t> data{
> > > > >                         static_cast<int32_t>(rects[0].x),

-- 
Regards,

Laurent Pinchart