From aa7109c148a025793da4ceeef6aa329eb24ff358 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 25 Oct 2018 10:42:49 +0000 Subject: [PATCH] Add copy constructors to the C++ Dawn interface This removes the need for Clone() so it is removed and also adds tests for the new constructors. BUG=dawn:11 Change-Id: Ia45c765c2d30e40b0e036427793a62327b2008fc Reviewed-on: https://dawn-review.googlesource.com/c/1901 Reviewed-by: Stephen White Reviewed-by: Kai Ninomiya Commit-Queue: Corentin Wallez --- examples/ComputeBoids.cpp | 4 +- generator/templates/apicpp.h | 27 +++++--- src/tests/DawnTest.cpp | 9 +-- src/tests/end2end/BindGroupTests.cpp | 4 +- .../end2end/ComputeCopyStorageBufferTests.cpp | 4 +- src/tests/end2end/PushConstantTests.cpp | 4 +- src/tests/unittests/ObjectBaseTests.cpp | 68 +++++++++++++++++-- 7 files changed, 90 insertions(+), 30 deletions(-) diff --git a/examples/ComputeBoids.cpp b/examples/ComputeBoids.cpp index 77b2ad8247..869f95f4b2 100644 --- a/examples/ComputeBoids.cpp +++ b/examples/ComputeBoids.cpp @@ -232,9 +232,9 @@ void initSim() { dawn::PipelineLayout pl = utils::MakeBasicPipelineLayout(device, &bgl); dawn::ComputePipelineDescriptor csDesc; - csDesc.module = module.Clone(); + csDesc.module = module; csDesc.entryPoint = "main"; - csDesc.layout = pl.Clone(); + csDesc.layout = pl; updatePipeline = device.CreateComputePipeline(&csDesc); dawn::BufferView updateParamsView = updateParams.CreateBufferViewBuilder() diff --git a/generator/templates/apicpp.h b/generator/templates/apicpp.h index 68873ac9d8..af142b04c0 100644 --- a/generator/templates/apicpp.h +++ b/generator/templates/apicpp.h @@ -70,19 +70,29 @@ namespace dawn { if (mHandle) Derived::DawnRelease(mHandle); } - ObjectBase(ObjectBase const& other) = delete; - Derived& operator=(ObjectBase const& other) = delete; + ObjectBase(ObjectBase const& other) + : ObjectBase(other.Get()) { + } + Derived& operator=(ObjectBase const& other) { + if (&other != this) { + if (mHandle) Derived::DawnRelease(mHandle); + mHandle = other.mHandle; + if (mHandle) Derived::DawnReference(mHandle); + } + + return static_cast(*this); + } ObjectBase(ObjectBase&& other) { mHandle = other.mHandle; other.mHandle = 0; } Derived& operator=(ObjectBase&& other) { - if (&other == this) return static_cast(*this); - - if (mHandle) Derived::DawnRelease(mHandle); - mHandle = other.mHandle; - other.mHandle = 0; + if (&other != this) { + if (mHandle) Derived::DawnRelease(mHandle); + mHandle = other.mHandle; + other.mHandle = 0; + } return static_cast(*this); } @@ -98,9 +108,6 @@ namespace dawn { mHandle = 0; return result; } - Derived Clone() const { - return Derived(mHandle); - } static Derived Acquire(CType handle) { Derived result; result.mHandle = handle; diff --git a/src/tests/DawnTest.cpp b/src/tests/DawnTest.cpp index ad88763214..f58dd2b95b 100644 --- a/src/tests/DawnTest.cpp +++ b/src/tests/DawnTest.cpp @@ -258,15 +258,13 @@ std::ostringstream& DawnTest::AddBufferExpectation(const char* file, uint32_t offset, uint32_t size, detail::Expectation* expectation) { - dawn::Buffer source = buffer.Clone(); - auto readback = ReserveReadback(size); // We need to enqueue the copy immediately because by the time we resolve the expectation, // the buffer might have been modified. dawn::CommandBuffer commands = device.CreateCommandBufferBuilder() - .CopyBufferToBuffer(source, offset, readback.buffer, readback.offset, size) + .CopyBufferToBuffer(buffer, offset, readback.buffer, readback.offset, size) .GetResult(); queue.Submit(1, &commands); @@ -296,7 +294,6 @@ std::ostringstream& DawnTest::AddTextureExpectation(const char* file, uint32_t level, uint32_t pixelSize, detail::Expectation* expectation) { - dawn::Texture source = texture.Clone(); uint32_t rowPitch = Align(width * pixelSize, kTextureRowPitchAlignment); uint32_t size = rowPitch * (height - 1) + width * pixelSize; @@ -306,7 +303,7 @@ std::ostringstream& DawnTest::AddTextureExpectation(const char* file, // the texture might have been modified. dawn::CommandBuffer commands = device.CreateCommandBufferBuilder() - .CopyTextureToBuffer(source, x, y, 0, width, height, 1, level, 0, readback.buffer, + .CopyTextureToBuffer(texture, x, y, 0, width, height, 1, level, 0, readback.buffer, readback.offset, rowPitch) .GetResult(); @@ -359,7 +356,7 @@ DawnTest::ReadbackReservation DawnTest::ReserveReadback(uint32_t readbackSize) { slot.buffer = device.CreateBuffer(&descriptor); ReadbackReservation reservation; - reservation.buffer = slot.buffer.Clone(); + reservation.buffer = slot.buffer; reservation.slot = mReadbackSlots.size(); reservation.offset = 0; diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp index 9510e0629b..8585151aec 100644 --- a/src/tests/end2end/BindGroupTests.cpp +++ b/src/tests/end2end/BindGroupTests.cpp @@ -52,9 +52,9 @@ TEST_P(BindGroupTests, ReusedBindGroupSingleSubmit) { dawn::ShaderModule module = utils::CreateShaderModule(device, dawn::ShaderStage::Compute, shader); dawn::ComputePipelineDescriptor cpDesc; - cpDesc.module = module.Clone(); + cpDesc.module = module; cpDesc.entryPoint = "main"; - cpDesc.layout = pl.Clone(); + cpDesc.layout = pl; dawn::ComputePipeline cp = device.CreateComputePipeline(&cpDesc); dawn::BufferDescriptor bufferDesc; diff --git a/src/tests/end2end/ComputeCopyStorageBufferTests.cpp b/src/tests/end2end/ComputeCopyStorageBufferTests.cpp index 9ee30213f2..d047ec185c 100644 --- a/src/tests/end2end/ComputeCopyStorageBufferTests.cpp +++ b/src/tests/end2end/ComputeCopyStorageBufferTests.cpp @@ -39,9 +39,9 @@ void ComputeCopyStorageBufferTests::BasicTest(const char* shader) { auto pl = utils::MakeBasicPipelineLayout(device, &bgl); dawn::ComputePipelineDescriptor csDesc; - csDesc.module = module.Clone(); + csDesc.module = module; csDesc.entryPoint = "main"; - csDesc.layout = pl.Clone(); + csDesc.layout = pl; dawn::ComputePipeline pipeline = device.CreateComputePipeline(&csDesc); // Set up src storage buffer diff --git a/src/tests/end2end/PushConstantTests.cpp b/src/tests/end2end/PushConstantTests.cpp index 070abeccab..80fa85777e 100644 --- a/src/tests/end2end/PushConstantTests.cpp +++ b/src/tests/end2end/PushConstantTests.cpp @@ -146,9 +146,9 @@ class PushConstantTest: public DawnTest { ); dawn::ComputePipelineDescriptor descriptor; - descriptor.module = module.Clone(); + descriptor.module = module; descriptor.entryPoint = "main"; - descriptor.layout = pl.Clone(); + descriptor.layout = pl; return device.CreateComputePipeline(&descriptor); } diff --git a/src/tests/unittests/ObjectBaseTests.cpp b/src/tests/unittests/ObjectBaseTests.cpp index 1e6058a24f..2c9dccbffb 100644 --- a/src/tests/unittests/ObjectBaseTests.cpp +++ b/src/tests/unittests/ObjectBaseTests.cpp @@ -22,7 +22,7 @@ class Object : public dawn::ObjectBase { using ObjectBase::operator=; static void DawnReference(int* handle) { - ASSERT_LT(0, *handle); + ASSERT_LE(0, *handle); *handle += 1; } static void DawnRelease(int* handle) { @@ -52,16 +52,14 @@ TEST(ObjectBase, AcquireConstruction) { ASSERT_EQ(0, refcount); } -// Test that cloning takes a new ref. Also test .Get(). -TEST(ObjectBase, Clone) { +// Test .Get(). +TEST(ObjectBase, Get) { int refcount = 1; { Object obj1(&refcount); - Object obj2 = obj1.Clone(); - ASSERT_EQ(3, refcount); + ASSERT_EQ(2, refcount); ASSERT_EQ(&refcount, obj1.Get()); - ASSERT_EQ(&refcount, obj2.Get()); } ASSERT_EQ(1, refcount); } @@ -91,6 +89,51 @@ TEST(ObjectBase, OperatorBool) { } } +// Test the copy constructor of C++ objects +TEST(ObjectBase, CopyConstructor) { + int refcount = 1; + + Object source(&refcount); + Object destination(source); + + ASSERT_EQ(source.Get(), &refcount); + ASSERT_EQ(destination.Get(), &refcount); + ASSERT_EQ(3, refcount); + + destination = Object(); + ASSERT_EQ(refcount, 2); +} + +// Test the copy assignment of C++ objects +TEST(ObjectBase, CopyAssignment) { + int refcount = 1; + Object source(&refcount); + + Object destination; + destination = source; + + ASSERT_EQ(source.Get(), &refcount); + ASSERT_EQ(destination.Get(), &refcount); + ASSERT_EQ(3, refcount); + + destination = Object(); + ASSERT_EQ(refcount, 2); +} + +// Test the copy assignment of C++ objects onto themselves +TEST(ObjectBase, CopyAssignmentSelf) { + int refcount = 1; + + Object obj(&refcount); + + // Fool the compiler to avoid a -Wself-assign-overload + Object* objPtr = &obj; + obj = *objPtr; + + ASSERT_EQ(obj.Get(), &refcount); + ASSERT_EQ(refcount, 2); +} + // Test the move constructor of C++ objects TEST(ObjectBase, MoveConstructor) { int refcount = 1; @@ -121,3 +164,16 @@ TEST(ObjectBase, MoveAssignment) { ASSERT_EQ(refcount, 1); } +// Test the move assignment of C++ objects onto themselves +TEST(ObjectBase, MoveAssignmentSelf) { + int refcount = 1; + + Object obj(&refcount); + + // Fool the compiler to avoid a -Wself-move + Object* objPtr = &obj; + obj = std::move(*objPtr); + + ASSERT_EQ(obj.Get(), &refcount); + ASSERT_EQ(refcount, 2); +}