Skip to content

Commit

Permalink
Explicitly delete copy constructor and assignment.
Browse files Browse the repository at this point in the history
Signed-off-by: Addisu Z. Taddese <[email protected]>
  • Loading branch information
azeey committed Nov 5, 2020
1 parent 4997432 commit d5e405a
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 111 deletions.
13 changes: 6 additions & 7 deletions include/sdf/Root.hh
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ namespace sdf
/// \brief Default constructor
public: Root();

/// \brief Copy constructor
/// \param[in] _root Root to copy.
public: Root(const Root &_root);
/// \brief Copy constructor is explicitly deleted to avoid copying the
/// FrameAttachedToGraph and PoseRelativeToGraphs contained in Root.
public: Root(const Root &_root) = delete;

/// \brief Move constructor
/// \param[in] _root Root to move.
Expand All @@ -69,10 +69,9 @@ namespace sdf
/// \return Reference to this.
public: Root &operator=(Root &&_root) noexcept;

/// \brief Copy assignment operator.
/// \param[in] _root Root to copy.
/// \return Reference to this.
public: Root &operator=(const Root &_root);
/// \brief Copy assignment operator is explicitly deleted to avoid copying
/// the FrameAttachedToGraph and PoseRelativeToGraphs contained in Root.
public: Root &operator=(const Root &_root) = delete;

/// \brief Destructor
public: ~Root();
Expand Down
35 changes: 0 additions & 35 deletions src/Root.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,47 +113,12 @@ Root::Root()
{
}

/////////////////////////////////////////////////
Root::Root(const Root &_root)
: dataPtr(new RootPrivate(*_root.dataPtr))
{
// TODO(addisu) Do we need to make deep copies of the graphs?
//
// By construction the sizes of the worlds vector, the
// worldFrameAttachedToGraphs vector and the worldPoseRelativeToGraphs vector
// should be the same.
for (std::size_t i = 0; i < this->dataPtr->worlds.size(); ++i)
{
this->dataPtr->worlds[i].SetFrameAttachedToGraph(
this->dataPtr->worldFrameAttachedToGraphs[i]);
this->dataPtr->worlds[i].SetPoseRelativeToGraph(
this->dataPtr->worldPoseRelativeToGraphs[i]);
}

// By construction the sizes of the models vector, the
// modelFrameAttachedToGraphs vector and the modelPoseRelativeToGraphs vector
// should be the same.
for (std::size_t i = 0; i < this->dataPtr->models.size(); ++i)
{
this->dataPtr->models[i].SetFrameAttachedToGraph(
this->dataPtr->modelFrameAttachedToGraphs[i]);
this->dataPtr->models[i].SetPoseRelativeToGraph(
this->dataPtr->modelPoseRelativeToGraphs[i]);
}
}

/////////////////////////////////////////////////
Root::Root(Root &&_root) noexcept
: dataPtr(std::exchange(_root.dataPtr, nullptr))
{
}

/////////////////////////////////////////////////
Root &Root::operator=(const Root &_root)
{
return *this = Root(_root);
}

/////////////////////////////////////////////////
Root &Root::operator=(Root &&_root) noexcept
{
Expand Down
71 changes: 2 additions & 69 deletions src/Root_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,34 +58,13 @@ TEST(DOMRoot, Construction)
EXPECT_TRUE(root.ActorByIndex(1) == nullptr);
}

/////////////////////////////////////////////////
TEST(DOMRoot, CopyConstructor)
{
sdf::Root root;
root.SetVersion("test_root");

sdf::Root root2(root);
EXPECT_EQ("test_root", root2.Version());
}

/////////////////////////////////////////////////
TEST(DOMRoot, CopyAssignmentOperator)
{
sdf::Root root;
root.SetVersion("test_root");

sdf::Root root2;
root2 = root;
EXPECT_EQ("test_root", root2.Version());
}

/////////////////////////////////////////////////
TEST(DOMRoot, MoveConstructor)
{
sdf::Root root;
root.SetVersion("test_root");

sdf::Root root2(root);
sdf::Root root2(std::move(root));
EXPECT_EQ("test_root", root2.Version());
}

Expand All @@ -100,25 +79,6 @@ TEST(DOMRoot, MoveAssignmentOperator)
EXPECT_EQ("test_root", root2.Version());
}

/////////////////////////////////////////////////
TEST(DOMRoot, CopyAssignmentAfterMove)
{
sdf::Root root1;
root1.SetVersion("root1");

sdf::Root root2;
root2.SetVersion("root2");

// This is similar to what std::swap does except it uses std::move for each
// assignment
sdf::Root tmp = std::move(root1);
root1 = root2;
root2 = tmp;

EXPECT_EQ("root2", root1.Version());
EXPECT_EQ("root1", root2.Version());
}

/////////////////////////////////////////////////
TEST(DOMRoot, StringParse)
{
Expand Down Expand Up @@ -205,7 +165,7 @@ TEST(DOMRoot, Set)
}

/////////////////////////////////////////////////
TEST(DOMRoot, FrameSemanticsOnCopyAndMove)
TEST(DOMRoot, FrameSemanticsOnMove)
{
const std::string sdfString1 = R"(
<sdf version="1.8">
Expand Down Expand Up @@ -265,33 +225,6 @@ TEST(DOMRoot, FrameSemanticsOnCopyAndMove)
testFrame2(root2);
}

{
sdf::Root root1;
sdf::Errors errors = root1.LoadSdfString(sdfString1);
EXPECT_TRUE(errors.empty()) << errors;
// root2 is copy constructed from root1
sdf::Root root2(root1);
testFrame1(root1);
testFrame1(root2);
}

{
sdf::Root root1;
sdf::Errors errors = root1.LoadSdfString(sdfString1);
EXPECT_TRUE(errors.empty()) << errors;
sdf::Root root2;
errors = root2.LoadSdfString(sdfString2);
EXPECT_TRUE(errors.empty()) << errors;

testFrame1(root1);
testFrame2(root2);

// root1 is copied into root2 via the assignment operator
root2 = root1;
testFrame1(root1);
testFrame1(root2);
}

{
sdf::Root root1;
sdf::Errors errors = root1.LoadSdfString(sdfString1);
Expand Down

0 comments on commit d5e405a

Please sign in to comment.