| Message ID | 20240430192620.779489-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | 77269a28692ce683416e4e47d304bc070a721a3b |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > C++20 deprecated implicit capture of `this` via `[=]`. > Fix that by explicitly capturing the necessary variables. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> It would be nice if there was a way to enable this warning even with C++17, but it's covered by -Wdeprecated :-S > --- > src/apps/cam/camera_session.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > index 334d2ed8..d1f65a99 100644 > --- a/src/apps/cam/camera_session.cpp > +++ b/src/apps/cam/camera_session.cpp > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > * Defer processing of the completed request to the event loop, to avoid > * blocking the camera manager thread. > */ > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > } > > void CameraSession::processRequest(Request *request)
Hi 2024. május 3., péntek 1:13 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > > C++20 deprecated implicit capture of `this` via `[=]`. > > Fix that by explicitly capturing the necessary variables. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > It would be nice if there was a way to enable this warning even with > C++17, but it's covered by -Wdeprecated :-S In the CI one could do builds with `meson setup ... -D cpp_std=c++20`. It almost builds, there is one other issue still: /libcamera/src/apps/qcam/main_window.cpp:193:38: error: bitwise operation between different enumeration types ‘Qt::Modifier’ and ‘Qt::Key’ is deprecated [-Werror=deprecated-enum-enum-conversion] 193 | action->setShortcut(Qt::CTRL | Qt::Key_Q); | ~~~~~~~~~^~~~~~~~~~~ > > > --- > > src/apps/cam/camera_session.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > index 334d2ed8..d1f65a99 100644 > > --- a/src/apps/cam/camera_session.cpp > > +++ b/src/apps/cam/camera_session.cpp > > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > > * Defer processing of the completed request to the event loop, to avoid > > * blocking the camera manager thread. > > */ > > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > > } > > > > void CameraSession::processRequest(Request *request) > > -- > Regards, > > Laurent Pinchart > Regards, Barnabás Pőcze
Hi Barnabás, On Thu, May 02, 2024 at 11:40:42PM +0000, Barnabás Pőcze wrote: > 2024. május 3., péntek 1:13 keltezéssel, Laurent Pinchart írta: > > On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > > > C++20 deprecated implicit capture of `this` via `[=]`. > > > Fix that by explicitly capturing the necessary variables. > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > It would be nice if there was a way to enable this warning even with > > C++17, but it's covered by -Wdeprecated :-S > > In the CI one could do builds with `meson setup ... -D cpp_std=c++20`. That's a good idea. > It almost builds, there is one other issue still: > > /libcamera/src/apps/qcam/main_window.cpp:193:38: error: bitwise operation between different enumeration types ‘Qt::Modifier’ and ‘Qt::Key’ is deprecated [-Werror=deprecated-enum-enum-conversion] > 193 | action->setShortcut(Qt::CTRL | Qt::Key_Q); > | ~~~~~~~~~^~~~~~~~~~~ I'm working on fixing that one :-) > > > --- > > > src/apps/cam/camera_session.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > > index 334d2ed8..d1f65a99 100644 > > > --- a/src/apps/cam/camera_session.cpp > > > +++ b/src/apps/cam/camera_session.cpp > > > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > > > * Defer processing of the completed request to the event loop, to avoid > > > * blocking the camera manager thread. > > > */ > > > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > > > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > > > } > > > > > > void CameraSession::processRequest(Request *request)
On Fri, May 03, 2024 at 02:58:45AM +0300, Laurent Pinchart wrote: > On Thu, May 02, 2024 at 11:40:42PM +0000, Barnabás Pőcze wrote: > > 2024. május 3., péntek 1:13 keltezéssel, Laurent Pinchart írta: > > > On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > > > > C++20 deprecated implicit capture of `this` via `[=]`. > > > > Fix that by explicitly capturing the necessary variables. > > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > It would be nice if there was a way to enable this warning even with > > > C++17, but it's covered by -Wdeprecated :-S > > > > In the CI one could do builds with `meson setup ... -D cpp_std=c++20`. > > That's a good idea. The result is surprising: https://gitlab.freedesktop.org/pinchartl/libcamera/-/jobs/58304138 I don't think I'll have time to investigate this very soon. > > It almost builds, there is one other issue still: > > > > /libcamera/src/apps/qcam/main_window.cpp:193:38: error: bitwise operation between different enumeration types ‘Qt::Modifier’ and ‘Qt::Key’ is deprecated [-Werror=deprecated-enum-enum-conversion] > > 193 | action->setShortcut(Qt::CTRL | Qt::Key_Q); > > | ~~~~~~~~~^~~~~~~~~~~ > > I'm working on fixing that one :-) The fix is on the list. > > > > --- > > > > src/apps/cam/camera_session.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > > > index 334d2ed8..d1f65a99 100644 > > > > --- a/src/apps/cam/camera_session.cpp > > > > +++ b/src/apps/cam/camera_session.cpp > > > > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > > > > * Defer processing of the completed request to the event loop, to avoid > > > > * blocking the camera manager thread. > > > > */ > > > > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > > > > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > > > > } > > > > > > > > void CameraSession::processRequest(Request *request)
Hi Barnabás On 01/05/24 12:56 am, Barnabás Pőcze wrote: > C++20 deprecated implicit capture of `this` via `[=]`. > Fix that by explicitly capturing the necessary variables. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Thank you for the patch. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> There is one more instance of "[=]" which I think is also a similar candidate... auto encoder = std::find_if(encoders.begin(), encoders.end(), [=](const Encoder &e) { return e.id() == encoderId; }); in src/apps/cam/drm.cpp It can be a patch squashed in this one, or a patch on top (I prefer the former though). > --- > src/apps/cam/camera_session.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > index 334d2ed8..d1f65a99 100644 > --- a/src/apps/cam/camera_session.cpp > +++ b/src/apps/cam/camera_session.cpp > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > * Defer processing of the completed request to the event loop, to avoid > * blocking the camera manager thread. > */ > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > } > > void CameraSession::processRequest(Request *request) >
Hi Umang, On Fri, May 03, 2024 at 04:24:47PM +0530, Umang Jain wrote: > Hi Barnabás > > On 01/05/24 12:56 am, Barnabás Pőcze wrote: > > C++20 deprecated implicit capture of `this` via `[=]`. > > Fix that by explicitly capturing the necessary variables. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > Thank you for the patch. > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > There is one more instance of "[=]" which I think is also a similar > candidate... > > auto encoder = std::find_if(encoders.begin(), > encoders.end(), > [=](const Encoder &e) { > return e.id() == > encoderId; > }); > > in src/apps/cam/drm.cpp That lambda function doesn't capture 'this'. 'e' is a parameter to the function, and 'encoderId' is a local variable. > It can be a patch squashed in this one, or a patch on top (I prefer the > former though). > > > --- > > src/apps/cam/camera_session.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > index 334d2ed8..d1f65a99 100644 > > --- a/src/apps/cam/camera_session.cpp > > +++ b/src/apps/cam/camera_session.cpp > > @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) > > * Defer processing of the completed request to the event loop, to avoid > > * blocking the camera manager thread. > > */ > > - EventLoop::instance()->callLater([=]() { processRequest(request); }); > > + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); > > } > > > > void CameraSession::processRequest(Request *request)
Hi, On 03/05/24 5:32 pm, Laurent Pinchart wrote: > Hi Umang, > > On Fri, May 03, 2024 at 04:24:47PM +0530, Umang Jain wrote: >> Hi Barnabás >> >> On 01/05/24 12:56 am, Barnabás Pőcze wrote: >>> C++20 deprecated implicit capture of `this` via `[=]`. >>> Fix that by explicitly capturing the necessary variables. >>> >>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >> Thank you for the patch. >> >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> >> There is one more instance of "[=]" which I think is also a similar >> candidate... >> >> auto encoder = std::find_if(encoders.begin(), >> encoders.end(), >> [=](const Encoder &e) { >> return e.id() == >> encoderId; >> }); >> >> in src/apps/cam/drm.cpp > That lambda function doesn't capture 'this'. 'e' is a parameter to the > function, and 'encoderId' is a local variable. Ah yes, that's correct. Got confused on looking at 'encoderId' Sorry for the noise. > >> It can be a patch squashed in this one, or a patch on top (I prefer the >> former though). >> >>> --- >>> src/apps/cam/camera_session.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>> index 334d2ed8..d1f65a99 100644 >>> --- a/src/apps/cam/camera_session.cpp >>> +++ b/src/apps/cam/camera_session.cpp >>> @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) >>> * Defer processing of the completed request to the event loop, to avoid >>> * blocking the camera manager thread. >>> */ >>> - EventLoop::instance()->callLater([=]() { processRequest(request); }); >>> + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); >>> } >>> >>> void CameraSession::processRequest(Request *request)
Hi 2024. május 3., péntek 5:12 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > On Fri, May 03, 2024 at 02:58:45AM +0300, Laurent Pinchart wrote: > > On Thu, May 02, 2024 at 11:40:42PM +0000, Barnabás Pőcze wrote: > > > 2024. május 3., péntek 1:13 keltezéssel, Laurent Pinchart írta: > > > > On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > > > > > C++20 deprecated implicit capture of `this` via `[=]`. > > > > > Fix that by explicitly capturing the necessary variables. > > > > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > It would be nice if there was a way to enable this warning even with > > > > C++17, but it's covered by -Wdeprecated :-S > > > > > > In the CI one could do builds with `meson setup ... -D cpp_std=c++20`. > > > > That's a good idea. > > The result is surprising: > > https://gitlab.freedesktop.org/pinchartl/libcamera/-/jobs/58304138 > > I don't think I'll have time to investigate this very soon. I have run into this as well, but it has "disappeared" after some time. Now I have taken a closer look. The issue appears to be that the "span" test creating a test executable named `span` in the `test` directory coupled with the `-I<build dir>/test` argument added by meson leads to the executable being included. $ g++ -Isubprojects/libcamera/test/camera-sensor.p -Isubprojects/libcamera/test [...] -c ../subprojects/libcamera/test/camera-sensor.cpp In file included from /usr/include/c++/13.2.1/format:44, from /usr/include/c++/13.2.1/bits/chrono_io.h:39, from /usr/include/c++/13.2.1/chrono:3370, from ../subprojects/libcamera/include/libcamera/base/utils.h:11, from ../subprojects/libcamera/test/camera-sensor.cpp:13: subprojects/libcamera/test/span:1:1: error: stray ‘\177’ in program 1 | <U+007F>ELF [...] > [...] Regards, Barnabás Pőcze
On Fri, May 03, 2024 at 05:25:17PM +0000, Barnabás Pőcze wrote: > Hi > > > 2024. május 3., péntek 5:12 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > > > On Fri, May 03, 2024 at 02:58:45AM +0300, Laurent Pinchart wrote: > > > On Thu, May 02, 2024 at 11:40:42PM +0000, Barnabás Pőcze wrote: > > > > 2024. május 3., péntek 1:13 keltezéssel, Laurent Pinchart írta: > > > > > On Tue, Apr 30, 2024 at 07:26:21PM +0000, Barnabás Pőcze wrote: > > > > > > C++20 deprecated implicit capture of `this` via `[=]`. > > > > > > Fix that by explicitly capturing the necessary variables. > > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > It would be nice if there was a way to enable this warning even with > > > > > C++17, but it's covered by -Wdeprecated :-S > > > > > > > > In the CI one could do builds with `meson setup ... -D cpp_std=c++20`. > > > > > > That's a good idea. > > > > The result is surprising: > > > > https://gitlab.freedesktop.org/pinchartl/libcamera/-/jobs/58304138 > > > > I don't think I'll have time to investigate this very soon. > > I have run into this as well, but it has "disappeared" after some time. Now I have > taken a closer look. The issue appears to be that the "span" test creating a test executable > named `span` in the `test` directory coupled with the `-I<build dir>/test` argument > added by meson leads to the executable being included. Ahhhhh... Thanks for investigating. This is the first time I find a real drawback of the extension-less headers for C++ :-( I'll try to post a fix. > $ g++ -Isubprojects/libcamera/test/camera-sensor.p -Isubprojects/libcamera/test [...] -c ../subprojects/libcamera/test/camera-sensor.cpp > In file included from /usr/include/c++/13.2.1/format:44, > from /usr/include/c++/13.2.1/bits/chrono_io.h:39, > from /usr/include/c++/13.2.1/chrono:3370, > from ../subprojects/libcamera/include/libcamera/base/utils.h:11, > from ../subprojects/libcamera/test/camera-sensor.cpp:13: > subprojects/libcamera/test/span:1:1: error: stray ‘\177’ in program > 1 | <U+007F>ELF [...] > > > [...]
diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp index 334d2ed8..d1f65a99 100644 --- a/src/apps/cam/camera_session.cpp +++ b/src/apps/cam/camera_session.cpp @@ -382,7 +382,7 @@ void CameraSession::requestComplete(Request *request) * Defer processing of the completed request to the event loop, to avoid * blocking the camera manager thread. */ - EventLoop::instance()->callLater([=]() { processRequest(request); }); + EventLoop::instance()->callLater([this, request]() { processRequest(request); }); } void CameraSession::processRequest(Request *request)
C++20 deprecated implicit capture of `this` via `[=]`. Fix that by explicitly capturing the necessary variables. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/apps/cam/camera_session.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)