Skip to content

Commit

Permalink
Promote -Wunqualified-std-cast-call to an error (RobotLocomotion#21760)
Browse files Browse the repository at this point in the history
Modify default compile flags to make -Wunqualified-std-cast-call an
error. Fix unqualified instances of std::move that were tripping this.
  • Loading branch information
mwoehlke-kitware authored Aug 1, 2024
1 parent b11d3c3 commit bb415eb
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 15 deletions.
28 changes: 14 additions & 14 deletions common/test/copyable_unique_ptr_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromCopyable) {
cup<CloneOnly> u_ptr(base_ptr = new CloneOnly(1));
EXPECT_EQ(u_ptr.get(), base_ptr);
// Move constructor on copyable_unique-ptr of same specialized class.
cup<CloneOnly> cup_ptr(move(u_ptr));
cup<CloneOnly> cup_ptr(std::move(u_ptr));
EXPECT_EQ(u_ptr.get(), nullptr);
EXPECT_EQ(cup_ptr.get(), base_ptr);

Expand All @@ -490,7 +490,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromCopyable) {
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(u_ptr2.get()));
// Copy constructor on copyable_unique-ptr of same specialized class, but
// contains derived class.
cup<CloneOnly> cup_ptr2(move(u_ptr2));
cup<CloneOnly> cup_ptr2(std::move(u_ptr2));
EXPECT_EQ(u_ptr2.get(), nullptr);
EXPECT_EQ(cup_ptr2.get(), co_ptr);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(cup_ptr2.get()));
Expand All @@ -499,7 +499,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromCopyable) {
CloneOnlyChildWithClone* co_ptr3;
cup<CloneOnlyChildWithClone> u_ptr3(co_ptr3 = new CloneOnlyChildWithClone(3));
EXPECT_EQ(u_ptr3.get(), co_ptr3);
cup<CloneOnly> cup_ptr3(move(u_ptr3));
cup<CloneOnly> cup_ptr3(std::move(u_ptr3));
EXPECT_EQ(u_ptr3.get(), nullptr);
EXPECT_EQ(cup_ptr3.get(), co_ptr3);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(cup_ptr3.get()));
Expand All @@ -515,7 +515,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromUnique) {
unique_ptr<CloneOnly> u_ptr(base_ptr = new CloneOnly(1));
EXPECT_EQ(u_ptr.get(), base_ptr);
// Move constructor on copyable_unique-ptr of same specialized class.
cup<CloneOnly> cup_ptr(move(u_ptr));
cup<CloneOnly> cup_ptr(std::move(u_ptr));
EXPECT_EQ(u_ptr.get(), nullptr);
EXPECT_EQ(cup_ptr.get(), base_ptr);

Expand All @@ -525,7 +525,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromUnique) {
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(u_ptr2.get()));
// Copy constructor on copyable_unique-ptr of same specialized class, but
// contains derived class.
cup<CloneOnly> cup_ptr2(move(u_ptr2));
cup<CloneOnly> cup_ptr2(std::move(u_ptr2));
EXPECT_EQ(u_ptr2.get(), nullptr);
EXPECT_EQ(cup_ptr2.get(), co_ptr);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(cup_ptr2.get()));
Expand All @@ -535,7 +535,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveConstructFromUnique) {
unique_ptr<CloneOnlyChildWithClone> u_ptr3(
co_ptr3 = new CloneOnlyChildWithClone(3));
EXPECT_EQ(u_ptr3.get(), co_ptr3);
cup<CloneOnly> cup_ptr3(move(u_ptr3));
cup<CloneOnly> cup_ptr3(std::move(u_ptr3));
EXPECT_EQ(u_ptr3.get(), nullptr);
EXPECT_EQ(cup_ptr3.get(), co_ptr3);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(cup_ptr3.get()));
Expand Down Expand Up @@ -851,14 +851,14 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromCopyableUniquePtr) {
// Case 1: Assign empty unique_ptr to empty cup.
EXPECT_EQ(tgt.get(), nullptr);
DestructorTracker::dtor_called = false;
tgt = move(empty);
tgt = std::move(empty);
EXPECT_FALSE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), nullptr);
EXPECT_EQ(empty.get(), nullptr);

// Case 2: Assign non-empty unique_ptr<Base> to empty cup<Base>.
DestructorTracker::dtor_called = false;
tgt = move(src);
tgt = std::move(src);
EXPECT_FALSE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), raw); // Tgt has taken ownership of raw.
EXPECT_EQ(src.get(), nullptr); // Src has been cleared.
Expand All @@ -867,7 +867,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromCopyableUniquePtr) {
DestructorTracker* raw2;
cup<DestructorTracker> src2(raw2 = new DestructorTracker(124));
DestructorTracker::dtor_called = false;
tgt = move(src2);
tgt = std::move(src2);
EXPECT_TRUE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), raw2); // Tgt has taken ownership of raw.
EXPECT_EQ(src2.get(), nullptr); // Src has been cleared.
Expand All @@ -877,7 +877,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromCopyableUniquePtr) {
CloneOnlyChildWithClone* derived_raw;
cup<CloneOnlyChildWithClone> derived_src(derived_raw =
new CloneOnlyChildWithClone(13));
base_tgt = move(derived_src);
base_tgt = std::move(derived_src);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(base_tgt.get()));
EXPECT_EQ(base_tgt.get(), derived_raw); // Tgt has taken ownership of raw.
EXPECT_EQ(derived_src.get(), nullptr); // Src has been cleared.
Expand Down Expand Up @@ -906,14 +906,14 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromUniquePtr) {
// Case 1: Assign empty unique_ptr to empty cup.
EXPECT_EQ(tgt.get(), nullptr);
DestructorTracker::dtor_called = false;
tgt = move(empty);
tgt = std::move(empty);
EXPECT_FALSE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), nullptr);
EXPECT_EQ(empty.get(), nullptr);

// Case 2: Assign non-empty unique_ptr<Base> to empty cup<Base>.
DestructorTracker::dtor_called = false;
tgt = move(src);
tgt = std::move(src);
EXPECT_FALSE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), raw); // Tgt has taken ownership of raw.
EXPECT_EQ(src.get(), nullptr); // Src has been cleared.
Expand All @@ -922,7 +922,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromUniquePtr) {
DestructorTracker* raw2;
cup<DestructorTracker> src2(raw2 = new DestructorTracker(124));
DestructorTracker::dtor_called = false;
tgt = move(src2);
tgt = std::move(src2);
EXPECT_TRUE(DestructorTracker::dtor_called);
EXPECT_EQ(tgt.get(), raw2); // Tgt has taken ownership of raw.
EXPECT_EQ(src2.get(), nullptr); // Src has been cleared.
Expand All @@ -932,7 +932,7 @@ GTEST_TEST(CopyableUniquePtrTest, MoveAssignFromUniquePtr) {
CloneOnlyChildWithClone* derived_raw;
unique_ptr<CloneOnlyChildWithClone> derived_src(
derived_raw = new CloneOnlyChildWithClone(13));
base_tgt = move(derived_src);
base_tgt = std::move(derived_src);
EXPECT_TRUE(is_dynamic_castable<CloneOnlyChildWithClone>(base_tgt.get()));
EXPECT_EQ(base_tgt.get(), derived_raw); // Tgt has taken ownership of raw.
EXPECT_EQ(derived_src.get(), nullptr); // Src has been cleared.
Expand Down
2 changes: 1 addition & 1 deletion common/test_utilities/test/is_memcpy_movable_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ using std::make_unique;
class MemcpyMovable {
public:
explicit MemcpyMovable(string name)
: name_{make_unique<string>(move(name))} {}
: name_{make_unique<string>(std::move(name))} {}
MemcpyMovable(const MemcpyMovable& m) : MemcpyMovable(*m.name_) {}
MemcpyMovable(MemcpyMovable&& m) = default;
bool operator==(const MemcpyMovable& m) const { return *name_ == *m.name_; }
Expand Down
1 change: 1 addition & 0 deletions tools/skylark/drake_cc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ CLANG_FLAGS = CXX_FLAGS + [
"-Werror=range-loop-analysis",
"-Werror=return-stack-address",
"-Werror=sign-compare",
"-Werror=unqualified-std-cast-call",
]

# The CLANG_VERSION_SPECIFIC_FLAGS will be enabled for all C++ rules in the
Expand Down

0 comments on commit bb415eb

Please sign in to comment.