Fix move ResizeableObject move constructor
It wasn't handling non-owned allocators correctly, which messed up using
the move constructor on aos StaticBuilders.
Change-Id: Ie751fd0a0c76db723b375aaec310de66b08a55b1
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/events/event_loop_param_test.cc b/aos/events/event_loop_param_test.cc
index f4f2fcc..d2d3aa9 100644
--- a/aos/events/event_loop_param_test.cc
+++ b/aos/events/event_loop_param_test.cc
@@ -133,6 +133,33 @@
EXPECT_TRUE(happened);
}
+// Tests that a static sender's Builder object can be moved safely.
+TEST_P(AbstractEventLoopTest, StaticBuilderMoveConstructor) {
+ auto loop1 = MakePrimary();
+
+ aos::Sender<TestMessageStatic> sender =
+ loop1->MakeSender<TestMessageStatic>("/test");
+ aos::Fetcher<TestMessage> fetcher = loop1->MakeFetcher<TestMessage>("/test");
+ std::optional<aos::Sender<TestMessageStatic>::StaticBuilder> moved_to_builder;
+ {
+ aos::Sender<TestMessageStatic>::StaticBuilder moved_from_builder =
+ sender.MakeStaticBuilder();
+ moved_to_builder.emplace(std::move(moved_from_builder));
+ }
+
+ loop1->OnRun([this, &moved_to_builder]() {
+ moved_to_builder.value()->set_value(200);
+ CHECK(moved_to_builder.value().builder()->Verify());
+ moved_to_builder.value().CheckOk(moved_to_builder.value().Send());
+ this->Exit();
+ });
+
+ ASSERT_FALSE(fetcher.Fetch());
+ Run();
+ ASSERT_TRUE(fetcher.Fetch());
+ EXPECT_EQ(200, fetcher->value());
+}
+
// Tests that watcher can receive messages from a sender, sent via SendDetached.
TEST_P(AbstractEventLoopTest, BasicSendDetached) {
auto loop1 = Make();
diff --git a/aos/flatbuffers/base.cc b/aos/flatbuffers/base.cc
index 697f837..8ad3b98 100644
--- a/aos/flatbuffers/base.cc
+++ b/aos/flatbuffers/base.cc
@@ -7,6 +7,31 @@
}
} // namespace
+ResizeableObject::ResizeableObject(ResizeableObject &&other)
+ : buffer_(other.buffer_),
+ parent_(other.parent_),
+ owned_allocator_(std::move(other.owned_allocator_)),
+ allocator_(other.allocator_) {
+ // At this stage in the move the move constructors of the inherited types have
+ // not yet been called, so we edit the state of the other object now so that
+ // when everything is moved over into the new objects they will have the
+ // correct pointers.
+ for (size_t index = 0; index < other.NumberOfSubObjects(); ++index) {
+ SubObject object = other.GetSubObject(index);
+ if (object.object != nullptr) {
+ object.object->parent_ = this;
+ }
+ }
+ other.buffer_ = {};
+ other.allocator_ = nullptr;
+ other.parent_ = nullptr;
+ // Sanity check that the std::unique_ptr move didn't reallocate/move memory
+ // around.
+ if (owned_allocator_.get() != nullptr) {
+ CHECK_EQ(owned_allocator_.get(), allocator_);
+ }
+}
+
bool ResizeableObject::InsertBytes(void *insertion_point, size_t bytes,
SetZero set_zero) {
// See comments on InsertBytes() declaration and in FixObjects()
diff --git a/aos/flatbuffers/base.h b/aos/flatbuffers/base.h
index f99dbb8..387dbc3 100644
--- a/aos/flatbuffers/base.h
+++ b/aos/flatbuffers/base.h
@@ -99,13 +99,7 @@
// Users do not end up using the move constructor; however, it is needed to
// handle the fact that a ResizeableObject may be a member of an std::vector
// in the various generated types.
- ResizeableObject(ResizeableObject &&other)
- : buffer_(other.buffer_),
- owned_allocator_(std::move(other.owned_allocator_)),
- allocator_(owned_allocator_.get()) {
- other.buffer_ = {};
- other.allocator_ = nullptr;
- }
+ ResizeableObject(ResizeableObject &&other);
// Required alignment of this object.
virtual size_t Alignment() const = 0;
// Offset from the start of buffer() to the actual start of the object in
diff --git a/aos/flatbuffers/static_flatbuffers_test.cc b/aos/flatbuffers/static_flatbuffers_test.cc
index 52fa01e..1846540 100644
--- a/aos/flatbuffers/static_flatbuffers_test.cc
+++ b/aos/flatbuffers/static_flatbuffers_test.cc
@@ -1094,4 +1094,58 @@
VerifyJson<::aos::testing::UseSchemaStatic>("{\n\n}");
}
+// Tests that we can use the move constructor on a Builder.
+TEST_F(StaticFlatbuffersTest, BuilderMoveConstructor) {
+ uint8_t buffer[Builder<TestTableStatic>::kBufferSize];
+ aos::fbs::SpanAllocator allocator({buffer, sizeof(buffer)});
+ Builder<TestTableStatic> builder_from(&allocator);
+ Builder<TestTableStatic> builder(std::move(builder_from));
+ TestTableStatic *object = builder.get();
+ object->set_scalar(123);
+ {
+ auto vector = object->add_vector_of_scalars();
+ ASSERT_TRUE(vector->emplace_back(4));
+ ASSERT_TRUE(vector->emplace_back(5));
+ }
+ {
+ auto string = object->add_string();
+ string->SetString("Hello, World!");
+ }
+ {
+ auto vector_of_strings = object->add_vector_of_strings();
+ auto sub_string = CHECK_NOTNULL(vector_of_strings->emplace_back());
+ ASSERT_TRUE(sub_string->emplace_back('D'));
+ }
+ { object->set_substruct({971, 254}); }
+ {
+ auto subtable = object->add_subtable();
+ subtable->set_foo(1234);
+ }
+ {
+ auto vector = object->add_vector_of_structs();
+ ASSERT_TRUE(vector->emplace_back({48, 67}));
+ ASSERT_TRUE(vector->emplace_back({118, 148}));
+ ASSERT_TRUE(vector->emplace_back({971, 973}));
+ // Max vector size is three; this should fail.
+ ASSERT_FALSE(vector->emplace_back({1114, 2056}));
+ // We don't have any extra space available.
+ ASSERT_FALSE(vector->reserve(4));
+ ASSERT_FALSE(vector->emplace_back({1114, 2056}));
+ }
+ {
+ auto vector = object->add_vector_of_tables();
+ auto subobject = vector->emplace_back();
+ subobject->set_foo(222);
+ }
+ {
+ auto subtable = object->add_included_table();
+ subtable->set_foo(included::TestEnum::B);
+ }
+ ASSERT_TRUE(builder.AsFlatbufferSpan().Verify());
+ VLOG(1) << aos::FlatbufferToJson(builder.AsFlatbufferSpan(),
+ {.multi_line = true});
+ VLOG(1) << AnnotateBinaries(test_schema_, builder.buffer());
+ TestMemory(builder.buffer());
+}
+
} // namespace aos::fbs::testing