fix: Compile fix for MSVC 2022 in C++20 mode#778
Conversation
jmachowinski
force-pushed
the
main
branch
2 times, most recently
from
a84273f to
3607e9c
Compare
jmachowinski
changed the title
| template <class CC, class DD> | ||
| static void* into_context(CC&& call, DD&& drop) { | ||
| auto obj = new Closure<C, D, R, Args...>(std::forward<CC>(call), std::forward<DD>(drop)); | ||
| auto obj = new Closure<std::remove_cv_t<std::remove_reference_t<CC>>, |
There was a problem hiding this comment.
this change prevents creating closures from callable references (more precisely the closure will no longer use reference but force a copy, which might not even be available), which breaks current behavior.
There was a problem hiding this comment.
You are correct this breaks the lvalue reference case.
If CC is a rvalue a copy / move is done anyway as _call should be a non reference type in this case.
I was thinking about a constexp if etc for the rvalue case, but I don't think it is worth it.
I reverted this, and went back to the initial try, which is to replace the lambdas by custom structs.
There was a problem hiding this comment.
Is there an explanation why simple std::move(d) for D = std::remove_reference_t<decltype(d)) for lambda does not work ?
| }; | ||
| using DroppableType = typename detail::closures::Droppable<VectorDeleter>; | ||
| auto drop = DroppableType::into_context(std::move(VectorDeleter(ptr))); | ||
| auto drop = DroppableType::into_context(std::move(VectorDeleter({.ptr = ptr}))); |
There was a problem hiding this comment.
this is not a valid C++17 code, aggregate initialization is only available, starting from C++20. Just into_context(std::move(VectorDeleter{ptr})) (or even just into_context(VectorDeleter{ptr})) should be good enough, the question stays though why std::move on lambda fails ?
There was a problem hiding this comment.
Damn, your are right this is also an c++20 feature...
I honestly don't know why the move fails on msvc. From my point of view it is valid, and gcc and clang are fine with it...
The compile error is also not really helpful on this one.. (see ros2/rmw_zenoh#968 (comment) for details).
There was a problem hiding this comment.
Hm, I'm wondering will something simple like auto&& d = [p = ptr]() mutable { delete p; }; solve the problem ?
There was a problem hiding this comment.
just tried it, nope...
There was a problem hiding this comment.
The ros2 CI has just enforced c++20 as minimum, so now the compiler error is also visible there in english :
https://ci.ros2.org/job/ci_windows/27622/console
Any more ideas on what to try ?
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
|
@DenisBiryukov91 I'm merging this to unblock the ROS CI and progress for Lyrical release. |
3 participants
related to ros2/rmw_zenoh#968
This fixed compilation errors with MSCV 2022 in C++20 mode.
Note, the minimum c++ standard will be raised to 20 for the L-Release.