Skip to content

Commit 08a9699

Browse files
Merge branch 'main' into houliston/priority-changes
2 parents e9e797c + 800752b commit 08a9699

7 files changed

Lines changed: 349 additions & 47 deletions

File tree

‎src/PowerPlant.hpp‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -311,24 +311,6 @@ class PowerPlant {
311311
FusionFunction::call(*this, data, std::forward<Arguments>(args)...);
312312
}
313313

314-
template <template <typename> class First, template <typename> class... Remainder, typename... Arguments>
315-
void emit_shared(Arguments&&... args) {
316-
317-
using Functions = std::tuple<First<void>, Remainder<void>...>;
318-
using ArgumentPack = decltype(std::forward_as_tuple(*this, std::forward<Arguments>(args)...));
319-
using CallerArgs = std::tuple<>;
320-
using FusionFunction = util::FunctionFusion<Functions, ArgumentPack, EmitCaller, CallerArgs, 1>;
321-
322-
// Provide a check to make sure they are passing us the right stuff
323-
static_assert(
324-
FusionFunction::value,
325-
"There was an error with the arguments for the emit function, Check that your scope and arguments "
326-
"match what you are trying to do.");
327-
328-
// Fuse our emit handlers and call the fused function
329-
FusionFunction::call(*this, std::forward<Arguments>(args)...);
330-
}
331-
332314
/// Our TaskScheduler that handles distributing task to the pool threads
333315
threading::scheduler::Scheduler scheduler;
334316
/// Our vector of Reactors, will get destructed when this vector is

‎src/Reactor.hpp‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,9 @@ namespace dsl {
135135
template <typename WatchdogGroup, typename RuntimeType>
136136
struct WatchdogServicer;
137137
template <typename WatchdogGroup, typename RuntimeType>
138-
WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data);
138+
std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data);
139139
template <typename WatchdogGroup>
140-
WatchdogServicer<WatchdogGroup, void> ServiceWatchdog();
140+
std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog();
141141
} // namespace emit
142142
} // namespace word
143143
} // namespace dsl
@@ -272,10 +272,7 @@ class Reactor {
272272

273273
/// @copydoc dsl::word::emit::ServiceWatchdog
274274
template <typename WatchdogGroup, typename... Arguments>
275-
auto ServiceWatchdog(Arguments&&... args)
276-
// THIS IS VERY IMPORTANT, the return type must be dependent on the function call
277-
// otherwise it won't check it's valid in SFINAE
278-
-> decltype(dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...)) {
275+
auto ServiceWatchdog(Arguments&&... args) {
279276
return dsl::word::emit::ServiceWatchdog<WatchdogGroup>(std::forward<Arguments>(args)...);
280277
}
281278

@@ -368,7 +365,7 @@ class Reactor {
368365
util::CallbackGenerator<DSL, Function>(std::forward<Function>(callback)));
369366

370367
// Get our tuple from binding our reaction
371-
auto tuple = DSL::bind(reaction, std::get<Index>(args)...);
368+
auto tuple = DSL::bind(reaction, std::move(std::get<Index>(args))...);
372369

373370
auto handle = threading::ReactionHandle(reaction);
374371
reactor.reaction_handles.push_back(handle);
@@ -379,7 +376,8 @@ class Reactor {
379376
}
380377

381378
public:
382-
Binder(Reactor& r, Arguments&&... args) : reactor(r), args(args...) {}
379+
template <typename... Args>
380+
Binder(Reactor& r, Args&&... args) : reactor(r), args(std::forward<Args>(args)...) {}
383381

384382
template <typename Label, typename Function>
385383
auto then(Label&& label, Function&& callback) {

‎src/dsl/word/emit/Watchdog.hpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ namespace dsl {
125125
* @return A WatchdogServicer object which will update the service time of the specified watchdog
126126
*/
127127
template <typename WatchdogGroup, typename RuntimeType>
128-
WatchdogServicer<WatchdogGroup, RuntimeType> ServiceWatchdog(RuntimeType&& data) {
129-
return WatchdogServicer<WatchdogGroup, RuntimeType>(std::forward<RuntimeType>(data));
128+
std::unique_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>> ServiceWatchdog(RuntimeType&& data) {
129+
return std::make_unique<WatchdogServicer<WatchdogGroup, RuntimeType>>(std::forward<RuntimeType>(data));
130130
}
131131

132132
/**
@@ -137,8 +137,8 @@ namespace dsl {
137137
* @return WatchdogServicer<WatchdogGroup, void>
138138
*/
139139
template <typename WatchdogGroup>
140-
WatchdogServicer<WatchdogGroup, void> ServiceWatchdog() {
141-
return WatchdogServicer<WatchdogGroup, void>();
140+
std::unique_ptr<WatchdogServicer<WatchdogGroup, void>> ServiceWatchdog() {
141+
return std::make_unique<WatchdogServicer<WatchdogGroup, void>>();
142142
}
143143

144144
/**
@@ -158,9 +158,9 @@ namespace dsl {
158158

159159
template <typename WatchdogGroup, typename RuntimeType>
160160
static void emit(const PowerPlant& /*powerplant*/,
161-
WatchdogServicer<WatchdogGroup, RuntimeType>& servicer) {
161+
const std::shared_ptr<WatchdogServicer<WatchdogGroup, RuntimeType>>& servicer) {
162162
// Update our service time
163-
servicer.service();
163+
servicer->service();
164164
}
165165
};
166166

‎src/util/FunctionFusion.hpp‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
2121
*/
2222

23-
#ifndef NUCLEAR_UTIL_FUNCTIONFUSION_HPP
24-
#define NUCLEAR_UTIL_FUNCTIONFUSION_HPP
23+
#ifndef NUCLEAR_UTIL_FUNCTION_FUSION_HPP
24+
#define NUCLEAR_UTIL_FUNCTION_FUSION_HPP
2525

2626
#include "Sequence.hpp"
2727
#include "tuplify.hpp"
@@ -51,8 +51,16 @@ namespace util {
5151
auto apply_function_fusion_call(const std::tuple<Arguments...>& args,
5252
const Sequence<Shared...>& /*shared*/,
5353
const Sequence<Selected...>& /*selected*/)
54-
-> decltype(Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...)) {
55-
return Function::call(std::get<Shared>(args)..., std::get<Selected>(args)...);
54+
-> decltype(Function::call(
55+
std::get<Shared>(args)...,
56+
static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...)) {
57+
58+
return Function::call(
59+
// By not moving/forwarding the shared arguments they will always end up as lvalues even if they were
60+
// rvalues originally. That allows them to be used in multiple function calls without the first one being
61+
// able to steal them via a move constructor etc
62+
std::get<Shared>(args)...,
63+
static_cast<std::tuple_element_t<Selected, std::tuple<Arguments...>>>(std::get<Selected>(args))...);
5664
}
5765

5866
/**
@@ -138,13 +146,14 @@ namespace util {
138146
*
139147
* @return the result of calling this specific function
140148
*/
141-
template <typename Function, int Start, int End>
149+
template <typename Function, int Start, int End, typename... Args>
142150
// It is forwarded as a tuple
143-
// NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
144-
static auto call_one(const Sequence<Start, End>& /*e*/, Arguments&&... args)
145-
-> decltype(apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...))) {
151+
static auto call_one(const Sequence<Start, End>& /*e*/, Args&&... args)
152+
-> decltype(apply_function_fusion_call<Function, Shared, Start, End>(
153+
std::forward_as_tuple(std::forward<Args>(args)...))) {
146154

147-
return apply_function_fusion_call<Function, Shared, Start, End>(std::forward_as_tuple(args...));
155+
return apply_function_fusion_call<Function, Shared, Start, End>(
156+
std::forward_as_tuple(std::forward<Args>(args)...));
148157
}
149158

150159
/**
@@ -177,12 +186,14 @@ namespace util {
177186
NextStep::call(std::forward<Args>(args)...))) {
178187

179188
// Call each on a separate line to preserve order of execution
180-
// TODO(thouliston) this is a legitimate bug, fix it in a future PR and add a test to ensure it doesn't
181-
// regress
182-
// NOLINTNEXTLINE(bugprone-use-after-move)
183-
auto current = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
189+
// The std::forwards here are fine even though they are passed to multiple functions.
190+
// As part of function fusion before, all of the shared arguments are enforced to be lvalues and only the
191+
// selected arguments will be potentially rvalues. These rvalues are then only passed to a single target
192+
// function, even if they are forwarded multiple times to the functions which select the right arguments.
193+
// As shown in the moving test, casting to an rvalue, but then not using it won't change the result.
184194
// NOLINTNEXTLINE(bugprone-use-after-move)
185-
auto remainder = NextStep::call(std::forward<Args>(args)...);
195+
auto current = tuplify(call_one<CurrentFunction>(CurrentRange(), std::forward<Args>(args)...));
196+
auto remainder = NextStep::call(std::forward<Args>(args)...); // NOLINT(bugprone-use-after-move)
186197

187198
return std::tuple_cat(std::move(current), std::move(remainder));
188199
}
@@ -365,4 +376,4 @@ namespace util {
365376
} // namespace util
366377
} // namespace NUClear
367378

368-
#endif // NUCLEAR_UTIL_FUNCTIONFUSION_HPP
379+
#endif // NUCLEAR_UTIL_FUNCTION_FUSION_HPP

‎src/util/Sequence.hpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#ifndef NUCLEAR_UTIL_SEQUENCE_HPP
2424
#define NUCLEAR_UTIL_SEQUENCE_HPP
2525

26+
#include <type_traits>
27+
2628
namespace NUClear {
2729
namespace util {
2830

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* MIT License
3+
*
4+
* Copyright (c) 2022 NUClear Contributors
5+
*
6+
* This file is part of the NUClear codebase.
7+
* See https://github.com/Fastcode/NUClear for further info.
8+
*
9+
* Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
10+
* documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
11+
* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
12+
* permit persons to whom the Software is furnished to do so, subject to the following conditions:
13+
*
14+
* The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
15+
* Software.
16+
*
17+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
18+
* WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
19+
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
20+
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
*/
22+
23+
#include "util/FunctionFusion.hpp"
24+
25+
#include <catch2/catch_test_macros.hpp>
26+
#include <tuple>
27+
#include <utility>
28+
#include <vector>
29+
30+
31+
// There are several linting rules from clang-tidy that will trigger in this code.
32+
// However as the point of these tests is to show what the behaviour is in these situations they are suppressed for this
33+
// file
34+
// NOLINTBEGIN(bugprone-use-after-move)
35+
36+
namespace { // Make everything here internal linkage
37+
38+
/**
39+
* This struct is used to test what is passed from FunctionFusion to the call function.
40+
*
41+
* It has four different versions of append to see which one is called
42+
* The ones that take rvalue references will move the arguments into new temporaries to ensure the original arguments
43+
* will be empty after the function runs.
44+
*/
45+
struct Appender {
46+
47+
/// rvalue/rvalue
48+
static std::vector<char> append(std::vector<char>&& x, std::vector<char>&& y) {
49+
const std::vector<char> x1 = std::move(x);
50+
const std::vector<char> y1 = std::move(y);
51+
52+
std::vector<char> out;
53+
out.push_back('r');
54+
out.push_back('r');
55+
out.insert(out.end(), x1.begin(), x1.end()); // HERE
56+
out.insert(out.end(), y1.begin(), y1.end());
57+
58+
return out;
59+
}
60+
61+
/// rvalue/lvalue
62+
static std::vector<char> append(std::vector<char>&& x, const std::vector<char>& y) {
63+
const std::vector<char> x1 = std::move(x);
64+
const auto& y1 = y;
65+
66+
std::vector<char> out;
67+
out.push_back('r');
68+
out.push_back('l');
69+
out.insert(out.end(), x1.begin(), x1.end());
70+
out.insert(out.end(), y1.begin(), y1.end());
71+
72+
return out;
73+
}
74+
75+
/// lvalue/rvalue
76+
static std::vector<char> append(const std::vector<char>& x, std::vector<char>&& y) {
77+
const auto& x1 = x;
78+
const std::vector<char> y1 = std::move(y);
79+
80+
std::vector<char> out;
81+
out.push_back('l');
82+
out.push_back('r');
83+
out.insert(out.end(), x1.begin(), x1.end()); // HERE
84+
out.insert(out.end(), y1.begin(), y1.end());
85+
86+
return out;
87+
}
88+
89+
/// lvalue/lvalue
90+
static std::vector<char> append(const std::vector<char>& x, const std::vector<char>& y) {
91+
const auto& x1 = x;
92+
const auto& y1 = y;
93+
94+
std::vector<char> out;
95+
out.push_back('l');
96+
out.push_back('l');
97+
out.insert(out.end(), x.begin(), x.end());
98+
out.insert(out.end(), y.begin(), y.end());
99+
100+
return out;
101+
}
102+
};
103+
104+
template <typename T>
105+
struct AppendCaller {
106+
template <typename... Args>
107+
static auto call(Args&&... args) -> decltype(T::append(std::forward<Args>(args)...)) {
108+
return T::append(std::forward<Args>(args)...);
109+
}
110+
};
111+
112+
template <int Shared, typename... Args>
113+
auto do_fusion(Args&&... args) {
114+
return NUClear::util::FunctionFusion<std::tuple<Appender, Appender>,
115+
decltype(std::forward_as_tuple(std::forward<Args>(args)...)),
116+
AppendCaller,
117+
std::tuple<>,
118+
Shared>::call(std::forward<Args>(args)...);
119+
}
120+
121+
} // namespace
122+
123+
SCENARIO("Shared arguments to FunctionFusion are not moved", "[util][FunctionFusion][shared][move]") {
124+
125+
WHEN("calling append with 1 shared and 1 selected arguments") {
126+
std::vector<char> shared({'s'});
127+
std::vector<char> arg1({'1'});
128+
std::vector<char> arg2({'2'});
129+
130+
auto result = do_fusion<1>(std::move(shared), std::move(arg1), std::move(arg2));
131+
132+
const auto& r1 = std::get<0>(result);
133+
const auto& r2 = std::get<1>(result);
134+
135+
THEN("the results are correct") {
136+
CHECK(r1 == std::vector<char>({'l', 'r', 's', '1'}));
137+
CHECK(r2 == std::vector<char>({'l', 'r', 's', '2'}));
138+
}
139+
THEN("the shared argument is not moved") {
140+
CHECK(shared == std::vector<char>({'s'}));
141+
}
142+
THEN("the selected arguments are moved") {
143+
CHECK(arg1.empty());
144+
CHECK(arg2.empty());
145+
}
146+
}
147+
148+
WHEN("calling append with 0 shared and 2 selected arguments") {
149+
std::vector<char> arg1({'1'});
150+
std::vector<char> arg2({'2'});
151+
std::vector<char> arg3({'3'});
152+
std::vector<char> arg4({'4'});
153+
154+
auto result = do_fusion<0>(std::move(arg1), std::move(arg2), std::move(arg3), std::move(arg4));
155+
156+
const auto& r1 = std::get<0>(result);
157+
const auto& r2 = std::get<1>(result);
158+
159+
THEN("the results are correct") {
160+
CHECK(r1 == std::vector<char>({'r', 'r', '1', '2'}));
161+
CHECK(r2 == std::vector<char>({'r', 'r', '3', '4'}));
162+
}
163+
THEN("the selected arguments are moved") {
164+
CHECK(arg1.empty());
165+
CHECK(arg2.empty());
166+
CHECK(arg3.empty());
167+
CHECK(arg4.empty());
168+
}
169+
}
170+
}
171+
172+
// NOLINTEND(bugprone-use-after-move)

0 commit comments

Comments
 (0)