From a181455dbbb2697a9bff3a2d0877b63ae38964f7 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Fri, 27 Dec 2024 18:30:39 +1100 Subject: [PATCH 01/14] Remove the no lint line to reactivate the linting error --- src/util/FunctionFusion.hpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/util/FunctionFusion.hpp b/src/util/FunctionFusion.hpp index 3dffc635..a9d9b03a 100644 --- a/src/util/FunctionFusion.hpp +++ b/src/util/FunctionFusion.hpp @@ -177,11 +177,7 @@ namespace util { NextStep::call(std::forward(args)...))) { // Call each on a separate line to preserve order of execution - // TODO(thouliston) this is a legitimate bug, fix it in a future PR and add a test to ensure it doesn't - // regress - // NOLINTNEXTLINE(bugprone-use-after-move) - auto current = tuplify(call_one(CurrentRange(), std::forward(args)...)); - // NOLINTNEXTLINE(bugprone-use-after-move) + auto current = tuplify(call_one(CurrentRange(), std::forward(args)...)); auto remainder = NextStep::call(std::forward(args)...); return std::tuple_cat(std::move(current), std::move(remainder)); From d02c1e419b3a2c392d01b25461b0fed339ee5419 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Fri, 27 Dec 2024 18:33:16 +1100 Subject: [PATCH 02/14] This really shouldn't have been nolinted, it should have been forwarded --- src/util/FunctionFusion.hpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/util/FunctionFusion.hpp b/src/util/FunctionFusion.hpp index a9d9b03a..da43a08e 100644 --- a/src/util/FunctionFusion.hpp +++ b/src/util/FunctionFusion.hpp @@ -20,8 +20,8 @@ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#ifndef NUCLEAR_UTIL_FUNCTIONFUSION_HPP -#define NUCLEAR_UTIL_FUNCTIONFUSION_HPP +#ifndef NUCLEAR_UTIL_FUNCTION_FUSION_HPP +#define NUCLEAR_UTIL_FUNCTION_FUSION_HPP #include "Sequence.hpp" #include "tuplify.hpp" @@ -140,11 +140,12 @@ namespace util { */ template // It is forwarded as a tuple - // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) static auto call_one(const Sequence& /*e*/, Arguments&&... args) - -> decltype(apply_function_fusion_call(std::forward_as_tuple(args...))) { + -> decltype(apply_function_fusion_call( + std::forward_as_tuple(std::forward(args)...))) { - return apply_function_fusion_call(std::forward_as_tuple(args...)); + return apply_function_fusion_call( + std::forward_as_tuple(std::forward(args)...)); } /** @@ -361,4 +362,4 @@ namespace util { } // namespace util } // namespace NUClear -#endif // NUCLEAR_UTIL_FUNCTIONFUSION_HPP +#endif // NUCLEAR_UTIL_FUNCTION_FUSION_HPP From bc8841e60922facd382a313e6a657c94e40211d5 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Fri, 27 Dec 2024 19:41:04 +1100 Subject: [PATCH 03/14] Add a quick test to see how rvalues/forwarding are actually handled by various platforms --- tests/tests/util/moving.cpp | 86 +++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 tests/tests/util/moving.cpp diff --git a/tests/tests/util/moving.cpp b/tests/tests/util/moving.cpp new file mode 100644 index 00000000..de630ddf --- /dev/null +++ b/tests/tests/util/moving.cpp @@ -0,0 +1,86 @@ +/* + * MIT License + * + * Copyright (c) 2022 NUClear Contributors + * + * This file is part of the NUClear codebase. + * See https://github.com/Fastcode/NUClear for further info. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated + * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE + * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +#include +#include + +namespace { // Internal linkage + +void do_nothing(std::vector&& v) { + // Do nothing with v +} + +void assign(std::vector&& v) { + std::vector v2 = v; +} + +void assign_with_move(std::vector&& v) { + std::vector v2 = std::move(v); +} + +template +void perfect_forwarding(T&& v) { + std::vector v2 = std::forward(v); +} + +} // namespace + +SCENARIO("Testing how rvalues are handled") { + GIVEN("a vector with some data") { + std::vector v1 = {0, 1}; + + WHEN("moving it to a function that does nothing") { + do_nothing(std::move(v1)); + THEN("the vector still has the data") { + CHECK(v1 == std::vector{0, 1}); + } + } + + WHEN("moving it to a function as an rvalue and assigns it to a new vector") { + assign(std::move(v1)); + THEN("the vector still has the data") { + REQUIRE(v1 == std::vector{0, 1}); + } + } + + WHEN("moving it to a function that assigns with perfect forwarding") { + perfect_forwarding(std::move(v1)); + THEN("the vector is empty") { + REQUIRE(v1 == std::vector{}); + } + } + + WHEN("moving it to a function as an rvalue and assigns it to a new vector with std::move") { + assign_with_move(std::move(v1)); + THEN("the vector is empty") { + REQUIRE(v1 == std::vector{}); + } + } + + WHEN("not moving it to a function with perfect forwarding") { + perfect_forwarding(v1); + THEN("the vector still has the data") { + REQUIRE(v1 == std::vector{0, 1}); + } + } + } +} From bf8ac584c76c6340d0dc3d537093db13382046f8 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Fri, 27 Dec 2024 19:51:03 +1100 Subject: [PATCH 04/14] Two more tests to make sure I understand --- tests/tests/util/moving.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/tests/util/moving.cpp b/tests/tests/util/moving.cpp index de630ddf..faacacb0 100644 --- a/tests/tests/util/moving.cpp +++ b/tests/tests/util/moving.cpp @@ -42,6 +42,16 @@ void perfect_forwarding(T&& v) { std::vector v2 = std::forward(v); } +template +void perfect_forwarding_to_assign(T&& v) { + assign(std::forward(v)); +} + +template +void perfect_forwarding_to_assign_with_move(T&& v) { + assign_with_move(std::forward(v)); +} + } // namespace SCENARIO("Testing how rvalues are handled") { @@ -82,5 +92,21 @@ SCENARIO("Testing how rvalues are handled") { REQUIRE(v1 == std::vector{0, 1}); } } + + WHEN("moving it to a function that assigns with perfect forwarding") { + perfect_forwarding_to_assign(std::move(v1)); + THEN("the vector is not empty") { + REQUIRE(v1 == std::vector{0, 1}); + } + } + + WHEN( + "moving it to a function as an rvalue and assigns it to a new vector with std::move with perfect " + "forwarding") { + perfect_forwarding_to_assign_with_move(std::move(v1)); + THEN("the vector is empty") { + REQUIRE(v1 == std::vector{}); + } + } } } From cfdca5c0ac059455bd43206823189324265fcc3f Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:03:45 +1100 Subject: [PATCH 05/14] As it was, the FunctionFusion was actually converting everything into an lvalue right before the call. The static cast here will fix the types so rvalues become rvalues again. Also remove the exception that watchdog servicer was using and make everything use unique pointers until there is something better --- src/Reactor.hpp | 11 ++++------- src/dsl/word/emit/Watchdog.hpp | 12 ++++++------ src/util/FunctionFusion.hpp | 6 ++++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/Reactor.hpp b/src/Reactor.hpp index 42095e90..51f8952b 100644 --- a/src/Reactor.hpp +++ b/src/Reactor.hpp @@ -135,9 +135,9 @@ namespace dsl { template struct WatchdogServicer; template - WatchdogServicer ServiceWatchdog(RuntimeType&& data); + std::unique_ptr> ServiceWatchdog(RuntimeType&& data); template - WatchdogServicer ServiceWatchdog(); + std::unique_ptr> ServiceWatchdog(); } // namespace emit } // namespace word } // namespace dsl @@ -272,10 +272,7 @@ class Reactor { /// @copydoc dsl::word::emit::ServiceWatchdog template - auto ServiceWatchdog(Arguments&&... args) - // THIS IS VERY IMPORTANT, the return type must be dependent on the function call - // otherwise it won't check it's valid in SFINAE - -> decltype(dsl::word::emit::ServiceWatchdog(std::forward(args)...)) { + auto ServiceWatchdog(Arguments&&... args) { return dsl::word::emit::ServiceWatchdog(std::forward(args)...); } @@ -368,7 +365,7 @@ class Reactor { util::CallbackGenerator(std::forward(callback))); // Get our tuple from binding our reaction - auto tuple = DSL::bind(reaction, std::get(args)...); + auto tuple = DSL::bind(reaction, std::move(std::get(args))...); auto handle = threading::ReactionHandle(reaction); reactor.reaction_handles.push_back(handle); diff --git a/src/dsl/word/emit/Watchdog.hpp b/src/dsl/word/emit/Watchdog.hpp index 6024ec59..f5754ad0 100644 --- a/src/dsl/word/emit/Watchdog.hpp +++ b/src/dsl/word/emit/Watchdog.hpp @@ -125,8 +125,8 @@ namespace dsl { * @return A WatchdogServicer object which will update the service time of the specified watchdog */ template - WatchdogServicer ServiceWatchdog(RuntimeType&& data) { - return WatchdogServicer(std::forward(data)); + std::unique_ptr> ServiceWatchdog(RuntimeType&& data) { + return std::make_unique>(std::forward(data)); } /** @@ -137,8 +137,8 @@ namespace dsl { * @return WatchdogServicer */ template - WatchdogServicer ServiceWatchdog() { - return WatchdogServicer(); + std::unique_ptr> ServiceWatchdog() { + return std::make_unique>(); } /** @@ -158,9 +158,9 @@ namespace dsl { template static void emit(const PowerPlant& /*powerplant*/, - WatchdogServicer& servicer) { + const std::shared_ptr>& servicer) { // Update our service time - servicer.service(); + servicer->service(); } }; diff --git a/src/util/FunctionFusion.hpp b/src/util/FunctionFusion.hpp index da43a08e..185768a4 100644 --- a/src/util/FunctionFusion.hpp +++ b/src/util/FunctionFusion.hpp @@ -48,11 +48,13 @@ namespace util { * @return The object returned by the called subfunction */ template - auto apply_function_fusion_call(const std::tuple& args, + auto apply_function_fusion_call(std::tuple&& args, const Sequence& /*shared*/, const Sequence& /*selected*/) -> decltype(Function::call(std::get(args)..., std::get(args)...)) { - return Function::call(std::get(args)..., std::get(args)...); + return Function::call( + static_cast>>(std::get(args))..., + static_cast>>(std::get(args))...); } /** From ac78f8758d7b39238c98d959000f2a245dad409c Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:14:47 +1100 Subject: [PATCH 06/14] forward arguments passed to bind (I don't think they can be anything other than stored as temporaries would be gone) --- src/Reactor.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Reactor.hpp b/src/Reactor.hpp index 51f8952b..d5fdab74 100644 --- a/src/Reactor.hpp +++ b/src/Reactor.hpp @@ -376,7 +376,8 @@ class Reactor { } public: - Binder(Reactor& r, Arguments&&... args) : reactor(r), args(args...) {} + template + Binder(Reactor& r, Args&&... args) : reactor(r), args(std::forward(args)...) {} template auto then(Label&& label, Function&& callback) { From 5f61c431d448a01247c51f4c2f6dbd31813ae0ad Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:15:07 +1100 Subject: [PATCH 07/14] Add a unit test to ensure that the arguments work as expected --- tests/tests/util/FunctionFusion.cpp | 158 ++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 tests/tests/util/FunctionFusion.cpp diff --git a/tests/tests/util/FunctionFusion.cpp b/tests/tests/util/FunctionFusion.cpp new file mode 100644 index 00000000..341d2d0c --- /dev/null +++ b/tests/tests/util/FunctionFusion.cpp @@ -0,0 +1,158 @@ +/* + * MIT License + * + * Copyright (c) 2022 NUClear Contributors + * + * This file is part of the NUClear codebase. + * See https://github.com/Fastcode/NUClear for further info. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated + * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE + * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +#include +#include +#include +#include + +#include "nuclear" + +namespace { // Make everything here internal linkage + +/** + * This struct is used to test what is passed from FunctionFusion to the call function. + * + * It has four different versions of append to see which one is called + * The ones that take rvalue references will move the arguments into new temporaries to ensure the original arguments + * will be empty after the function runs. + */ +struct Appender { + + /// rvalue/rvalue + static std::vector append(std::vector&& x, std::vector&& y) { + std::vector x1 = std::move(x); + std::vector y1 = std::move(y); + + std::vector out = {'r', 'r'}; + out.insert(out.end(), x1.begin(), x1.end()); + out.insert(out.end(), y1.begin(), y1.end()); + + return out; + } + + /// rvalue/lvalue + static std::vector append(std::vector&& x, const std::vector& y) { + std::vector x1 = std::move(x); + const auto& y1 = y; + + std::vector out = {'l', 'r'}; + out.insert(out.end(), x1.begin(), x1.end()); + out.insert(out.end(), y1.begin(), y1.end()); + + return out; + } + + /// lvalue/rvalue + static std::vector append(const std::vector& x, std::vector&& y) { + const auto& x1 = x; + std::vector y1 = std::move(y); + + std::vector out = {'l', 'r'}; + out.insert(out.end(), x1.begin(), x1.end()); + out.insert(out.end(), y1.begin(), y1.end()); + + return out; + } + + /// lvalue/lvalue + static std::vector append(const std::vector& x, const std::vector& y) { + const auto& x1 = x; + const auto& y1 = y; + + std::vector out = {'l', 'l'}; + out.insert(out.end(), x.begin(), x.end()); + out.insert(out.end(), y.begin(), y.end()); + + return out; + } +}; + +template +struct AppendCaller { + template + static auto call(Args&&... args) -> decltype(T::append(std::forward(args)...)) { + + std::cout << "Received: " << NUClear::util::demangle(typeid(std::tuple).name()) << std::endl; + return T::append(std::forward(args)...); + } +}; + +template +auto do_fusion(Args&&... args) { + return NUClear::util::FunctionFusion, + decltype(std::forward_as_tuple(std::forward(args)...)), + AppendCaller, + std::tuple<>, + Shared>::call(std::forward(args)...); +} + +} // namespace + +SCENARIO("Shared arguments to FunctionFusion are not moved", "[util][FunctionFusion][shared][move]") { + + WHEN("calling append with 1 shared and 1 selected arguments") { + std::vector shared({'s'}); + std::vector arg1({'1'}); + std::vector arg2({'2'}); + + auto result = do_fusion<1>(std::move(shared), std::move(arg1), std::move(arg2)); + + const auto& r1 = std::get<0>(result); + const auto& r2 = std::get<1>(result); + + THEN("the results are correct") { + CHECK(r1 == std::vector({'l', 'r', 's', '1'})); + CHECK(r2 == std::vector({'l', 'r', 's', '2'})); + } + THEN("the shared argument is not moved") { + CHECK(shared == std::vector({'s'})); + } + THEN("the selected arguments are moved") { + CHECK(arg1.empty()); + CHECK(arg2.empty()); + } + } + + WHEN("calling append with 0 shared and 2 selected arguments") { + std::vector arg1({'1'}); + std::vector arg2({'2'}); + std::vector arg3({'3'}); + std::vector arg4({'4'}); + + auto result = do_fusion<0>(std::move(arg1), std::move(arg2), std::move(arg3), std::move(arg4)); + + const auto& r1 = std::get<0>(result); + const auto& r2 = std::get<1>(result); + + THEN("the results are correct") { + CHECK(r1 == std::vector({'r', 'r', '1', '2'})); + CHECK(r2 == std::vector({'r', 'r', '3', '4'})); + } + THEN("the selected arguments are moved") { + CHECK(arg1.empty()); + CHECK(arg2.empty()); + CHECK(arg3.empty()); + CHECK(arg4.empty()); + } + } +} From 7fa3c889ed1ea88d540436b932aa87751809503e Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:40:46 +1100 Subject: [PATCH 08/14] Add comments and fix the shared arguments to be lvalues --- src/util/FunctionFusion.hpp | 18 +++++++++++++++--- tests/tests/util/FunctionFusion.cpp | 2 -- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/util/FunctionFusion.hpp b/src/util/FunctionFusion.hpp index 185768a4..b5712129 100644 --- a/src/util/FunctionFusion.hpp +++ b/src/util/FunctionFusion.hpp @@ -51,9 +51,15 @@ namespace util { auto apply_function_fusion_call(std::tuple&& args, const Sequence& /*shared*/, const Sequence& /*selected*/) - -> decltype(Function::call(std::get(args)..., std::get(args)...)) { + -> decltype(Function::call( + std::get(args)..., + static_cast>>(std::get(args))...)) { + return Function::call( - static_cast>>(std::get(args))..., + // By not moving/forwarding the shared arguments they will always end up as lvalues even if they were + // rvalues originally. That allows them to be used in multiple function calls without the first one being + // able to steal them via a move constructor etc + std::get(args)..., static_cast>>(std::get(args))...); } @@ -180,8 +186,14 @@ namespace util { NextStep::call(std::forward(args)...))) { // Call each on a separate line to preserve order of execution + // The std::forwards here are fine even though they are passed to multiple functions. + // As part of function fusion before, all of the shared arguments are enforced to be lvalues and only the + // selected arguments will be potentially rvalues. These rvalues are then only passed to a single target + // function, even if they are forwarded multiple times to the functions which select the right arguments. + // As shown in the moving test, casting to an rvalue, but then not using it won't change the result. + // NOLINTNEXTLINE(bugprone-use-after-move) auto current = tuplify(call_one(CurrentRange(), std::forward(args)...)); - auto remainder = NextStep::call(std::forward(args)...); + auto remainder = NextStep::call(std::forward(args)...); // NOLINT(bugprone-use-after-move) return std::tuple_cat(std::move(current), std::move(remainder)); } diff --git a/tests/tests/util/FunctionFusion.cpp b/tests/tests/util/FunctionFusion.cpp index 341d2d0c..d06bf461 100644 --- a/tests/tests/util/FunctionFusion.cpp +++ b/tests/tests/util/FunctionFusion.cpp @@ -91,8 +91,6 @@ template struct AppendCaller { template static auto call(Args&&... args) -> decltype(T::append(std::forward(args)...)) { - - std::cout << "Received: " << NUClear::util::demangle(typeid(std::tuple).name()) << std::endl; return T::append(std::forward(args)...); } }; From 233ac915cf6ea73f7079dfe23f6ae767ab803c22 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:40:53 +1100 Subject: [PATCH 09/14] comment for the moving test --- tests/tests/util/moving.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/tests/util/moving.cpp b/tests/tests/util/moving.cpp index faacacb0..5b47b4c2 100644 --- a/tests/tests/util/moving.cpp +++ b/tests/tests/util/moving.cpp @@ -54,7 +54,24 @@ void perfect_forwarding_to_assign_with_move(T&& v) { } // namespace -SCENARIO("Testing how rvalues are handled") { +/** + * This test is mostly here to encode an understanding of l/r values in code. + * + * When a function receives a value as an "rvalue reference" it is still an lvalue as it has a name even though it's an + * lvalue reference to an rvalue. To make it back into an rvalue reference you need to do a static cast to an rvalue. + * Typically this is done with std::move or std::forward. + * + * These casts don't actually do anything to the underlying data, they just change the type of the reference. + * So if you do one of these casts, but then don't actually do anything with the reference, the data will still be in + * the same state as it was before the cast. + * + * As a result, if you std::move a variable into a function but that function doesn't do anything with the variable, the + * result will be no change to the variable. + * + * Therefore if you are std::forwarding variables and you know that the variables will not be used in multiple places + * (e.g. function fusion) then these casts won't cause problems. + */ +SCENARIO("Test to ensure that l and r values operate according to the standard") { GIVEN("a vector with some data") { std::vector v1 = {0, 1}; From d5d3ad49b9e0d877e67a8b53f8a03586800655b3 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sat, 28 Dec 2024 17:41:10 +1100 Subject: [PATCH 10/14] Remove the weird non data emit --- src/PowerPlant.hpp | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/PowerPlant.hpp b/src/PowerPlant.hpp index f0601f41..afcf9d93 100644 --- a/src/PowerPlant.hpp +++ b/src/PowerPlant.hpp @@ -311,24 +311,6 @@ class PowerPlant { FusionFunction::call(*this, data, std::forward(args)...); } - template