Skip to content

[clang-tidy] Make bugprone-unhandled-self-assignment check more general #147066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,22 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl(
hasName("operator="), ofClass(equalsBoundNode("class"))))))));

// Checking that some kind of constructor is called and followed by a `swap`:
// T& operator=(const T& other) {
// T tmp{this->internal_data(), some, other, args};
// swap(tmp);
// return *this;
// }
const auto HasCopyAndSwap = cxxMethodDecl(
ofClass(cxxRecordDecl(unless(hasAncestor(classTemplateDecl())))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is hasAncestor necessary here? Could it just be hasParent? Or why skip templates at all?

hasDescendant(
stmt(hasDescendant(
varDecl(hasType(cxxRecordDecl(equalsBoundNode("class"))))
.bind("tmp_var")),
hasDescendant(callExpr(callee(functionDecl(hasName("swap"))),
hasAnyArgument(declRefExpr(to(varDecl(
equalsBoundNode("tmp_var"))))))))));
Comment on lines +84 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this matching can be a bit stronger. If swap is a free function, you'd want *this to be one of the args, and if it is a member function, then the matcher can just match a cxxMemberCallExpr. E.g., roughly

anyOf(cxxMemberCallExpr(hasArgument(0, declRefExpr(to(varDecl(
                                          equalsBoundNode("tmp_var")))))), 
      functionDecl(argumentCountIs(2), hasName("swap"),
          hasAnyArgument(declRefExpr(to(varDecl(
                                          equalsBoundNode("tmp_var")))))
          hasAnyArgument(unaryOperator(has(cxxThisExpr()), hasOperatorName("*")))))))

Otherwise, I could create two temporaries of the class and swap only them.


DeclarationMatcher AdditionalMatcher = cxxMethodDecl();
if (WarnOnlyIfThisHasSuspiciousField) {
// Matcher for standard smart pointers.
Expand All @@ -89,14 +105,14 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
hasType(arrayType())))))));
}

Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
isCopyAssignmentOperator(), IsUserDefined,
HasReferenceParam, HasNoSelfCheck,
unless(HasNonTemplateSelfCopy),
unless(HasTemplateSelfCopy),
HasNoNestedSelfAssign, AdditionalMatcher)
.bind("copyAssignmentOperator"),
this);
Finder->addMatcher(
cxxMethodDecl(
ofClass(cxxRecordDecl().bind("class")), isCopyAssignmentOperator(),
IsUserDefined, HasReferenceParam, HasNoSelfCheck,
unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy),
unless(HasCopyAndSwap), HasNoNestedSelfAssign, AdditionalMatcher)
.bind("copyAssignmentOperator"),
this);
}

void UnhandledSelfAssignmentCheck::check(
Expand Down
6 changes: 6 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@ Changes in existing checks
calls of ``std::string`` constructor with char pointer, start position and
length parameters.

- Improved :doc:`bugprone-unhandled-self-assignment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be after bugprone-unchecked-optional-access

<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by adding
an additional matcher that generalizes the copy-and-swap idiom pattern
detection. The checker now properly recognizes copy-and-swap implementations
that use "extended" copy/move constructors.
Comment on lines +195 to +196
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the second sentence is needed


- Improved :doc:`bugprone-unchecked-optional-access
<clang-tidy/checks/bugprone/unchecked-optional-access>` fixing false
positives from smart pointer accessors repeated in checking ``has_value``
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ template <class T>
class auto_ptr {
};

namespace pmr {
template <typename TYPE = void>
class allocator {};
}

struct allocator_arg_t {} allocator_arg;

} // namespace std

void assert(int expression){};
Expand Down Expand Up @@ -540,6 +547,89 @@ class NotACopyAssignmentOperator {
Uy *getUy() const { return Ptr2; }
};

// Support "extended" copy/move constructors
class AllocatorAwareClass {
// pointer member to trigger bugprone-unhandled-self-assignment
void *foo = nullptr;

public:
using allocator_type = std::pmr::allocator<>;

AllocatorAwareClass(const AllocatorAwareClass& other) {
}

AllocatorAwareClass(const AllocatorAwareClass& other, const allocator_type& alloc) {
}

AllocatorAwareClass& operator=(const AllocatorAwareClass& other) {
AllocatorAwareClass tmp(other, get_allocator());
swap(tmp);
return *this;
}

void swap(AllocatorAwareClass& other) noexcept {
}

allocator_type get_allocator() const {
return allocator_type();
}
};

// Support "extended" copy/move constructors + std::swap
class AllocatorAwareClassStdSwap {
// pointer member to trigger bugprone-unhandled-self-assignment
void *foo = nullptr;

public:
using allocator_type = std::pmr::allocator<>;

AllocatorAwareClassStdSwap(const AllocatorAwareClassStdSwap& other) {
}

AllocatorAwareClassStdSwap(const AllocatorAwareClassStdSwap& other, const allocator_type& alloc) {
}

AllocatorAwareClassStdSwap& operator=(const AllocatorAwareClassStdSwap& other) {
using std::swap;

AllocatorAwareClassStdSwap tmp(other, get_allocator());
swap(*this, tmp);
return *this;
}

allocator_type get_allocator() const {
return allocator_type();
}
};

// Support "extended" copy/move constructors + ADL swap
class AllocatorAwareClassAdlSwap {
// pointer member to trigger bugprone-unhandled-self-assignment
void *foo = nullptr;

public:
using allocator_type = std::pmr::allocator<>;

AllocatorAwareClassAdlSwap(const AllocatorAwareClassAdlSwap& other) {
}

AllocatorAwareClassAdlSwap(const AllocatorAwareClassAdlSwap& other, const allocator_type& alloc) {
}

AllocatorAwareClassAdlSwap& operator=(const AllocatorAwareClassAdlSwap& other) {
AllocatorAwareClassAdlSwap tmp(other, get_allocator());
swap(*this, tmp);
return *this;
}

allocator_type get_allocator() const {
return allocator_type();
}

friend void swap(AllocatorAwareClassAdlSwap&, AllocatorAwareClassAdlSwap&) {
}
};

///////////////////////////////////////////////////////////////////
/// Test cases which should be caught by the check.

Expand Down