Skip to content

Commit

Permalink
[common] DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN detects mis-uses
Browse files Browse the repository at this point in the history
Add unit test advice to document and complement the new checking.
Upgrade yaml_node_test with an example of testing for move efficiency.
Fix name_value to correctly document its (pre-existing) non-copyable behavior.
  • Loading branch information
jwnimmer-tri committed Oct 10, 2021
1 parent 29e2ff5 commit ea20cde
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 25 deletions.
24 changes: 19 additions & 5 deletions common/drake_copyable.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,28 @@ class Foo {
// ...
};
</pre>
The macro contains a built-in self-check that copy-assignment is well-formed.
This self-check proves that the Classname is CopyConstructible, CopyAssignable,
MoveConstructible, and MoveAssignable (in all but the most arcane cases where a
member of the Classname is somehow CopyAssignable but not CopyConstructible).
Therefore, classes that use this macro typically will not need to have any unit
tests that check for the presence nor correctness of these functions.
However, the self-check does not provide any checks of the runtime efficiency
of the functions. If it is important for performance that the move functions
actually move (instead of making a copy), then you should consider capturing
that in a unit test.
*/
#define DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Classname) \
Classname(const Classname&) = default; \
Classname& operator=(const Classname&) = default; \
Classname(Classname&&) = default; \
Classname& operator=(Classname&&) = default; \
/* Fails at compile-time if default-copy doesn't work. */ \
static void DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE() { \
(void) static_cast<Classname& (Classname::*)( \
const Classname&)>(&Classname::operator=); \
}
/* Fails at compile-time if copy-assign doesn't compile. */ \
static void DrakeDefaultCopyAndMoveAndAssign_DoAssign( \
Classname* a, const Classname& b) { *a = b; } \
static_assert( \
&DrakeDefaultCopyAndMoveAndAssign_DoAssign == \
&DrakeDefaultCopyAndMoveAndAssign_DoAssign, \
"The given Classname is not default copy-asignable.");
2 changes: 1 addition & 1 deletion common/name_value.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ namespace drake {
template <typename T>
class NameValue {
public:
DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(NameValue)
DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(NameValue)

/// Type of the referenced value.
typedef T value_type;
Expand Down
1 change: 1 addition & 0 deletions common/yaml/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ drake_cc_googletest(
deps = [
":yaml_node",
"//common/test_utilities:expect_throws_message",
"//common/test_utilities:limit_malloc",
],
)

Expand Down
45 changes: 27 additions & 18 deletions common/yaml/test/yaml_node_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <gtest/gtest.h>

#include "drake/common/test_utilities/expect_throws_message.h"
#include "drake/common/test_utilities/limit_malloc.h"

namespace drake {
namespace yaml {
Expand All @@ -30,24 +31,6 @@ GTEST_TEST(YamlNodeTest, DefaultConstructor) {
EXPECT_EQ(dut.GetScalar(), "");
}

// Sanity check of defaulted operators. We don't need to test them
// exhaustively, because they are defaulted.
GTEST_TEST(YamlNodeTest, DefaultCopyMove) {
Node dut = Node::MakeScalar("foo");

// Copy constructor.
Node foo(dut);
EXPECT_EQ(dut.GetScalar(), "foo");
EXPECT_EQ(foo.GetScalar(), "foo");

// Move constructor.
Node bar(std::move(dut));
EXPECT_EQ(bar.GetScalar(), "foo");
// It is important for performance that the move constructor actually moves
// the stored data, instead of copying it.
EXPECT_EQ(dut.GetScalar(), "");
}

// Parameterize the remainder of the tests across the three possible types.
using Param = std::tuple<NodeType, std::string_view>;
class YamlNodeParamaterizedTest : public testing::TestWithParam<Param> {
Expand Down Expand Up @@ -125,6 +108,32 @@ TEST_P(YamlNodeParamaterizedTest, GetSetTag) {
EXPECT_EQ(dut.GetTag(), "tag");
}

// It is important for our YAML subsystem performance that the Node's move
// operations actually move the stored data, instead of copying it.
TEST_P(YamlNodeParamaterizedTest, EfficientMoveConstructor) {
Node dut = MakeNonEmptyDut();

auto guard = std::make_unique<test::LimitMalloc>();
Node foo(std::move(dut));
guard.reset();

EXPECT_EQ(dut, MakeEmptyDut());
EXPECT_EQ(foo, MakeNonEmptyDut());
}

// Ditto per the prior test case.
TEST_P(YamlNodeParamaterizedTest, EfficientMoveAssignment) {
Node dut = MakeNonEmptyDut();
Node foo;

auto guard = std::make_unique<test::LimitMalloc>();
foo = std::move(dut);
guard.reset();

EXPECT_EQ(dut, MakeEmptyDut());
EXPECT_EQ(foo, MakeNonEmptyDut());
}

// Check (non-)equality as affected by the stored type of empty nodes.
TEST_P(YamlNodeParamaterizedTest, EqualityPerType) {
Node dut = MakeEmptyDut();
Expand Down
2 changes: 1 addition & 1 deletion tools/workspace/pybind11/mkdoc.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@

# 'Broadphase' culling; do not recurse inside these symbols.
SKIP_RECURSE_NAMES = [
'DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE',
'DrakeDefaultCopyAndMoveAndAssign_DoAssign',
'Eigen',
'detail',
'dev',
Expand Down

0 comments on commit ea20cde

Please sign in to comment.