clang-tidy: bugprone-unhandled-self-assignment false positive

[kalinnikovv]

New bugprone-unhandled-self-assignment check provides false positives in some cases. It seems to check only for the presence of specific constructs which makes it less useful.

Example code with explanation:

template<typename T>
struct IntrusivePointer
{
    T* p = nullptr;

    IntrusivePointer& operator=(const IntrusivePointer& other) // warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
    {
        if (p != other.p) // however, on self-assignment p will be equal to other.p so this block will be skipped
        {
            T* old = p;
            p = other.p;
            ref(p);
            deref(old);
        }
        return *this;
    }

    static ref(T* p);
    static deref(T* p);
};

Adding if (this == &other) is redundant in this case, and using copy-and-swap idiom is basically equiavalent to removing the if altogether.

[llvmbot]

@llvm/issue-subscribers-bug

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[kalinnikovv]

More complete code example:

struct S {
    std::atomic_int refCtr;
};

template<typename T>
struct IntrusivePointer
{
    T* p = nullptr;

    IntrusivePointer(const IntrusivePointer& other)
        : p(other.p)
    {
        ref(p);
    }

    ~IntrusivePointer()
    {
        deref(p);
    }

    IntrusivePointer& operator=(const IntrusivePointer& other)
    {
        if (p != other.p)
        {
            ref(other.p);
            T* old = std::exchange(p, other.p);
            deref(old);
        }
        return *this;
    }

    void swap(IntrusivePointer& other)
    {
         std::swap(p, other.p);
    }

    static ref(T* p)
    {
        if (p)
        {
           ++p->refCtr;
        }
    }

    static deref(T* p)
    {
        if (p && !--p->refCtr)
        {
            delete p;
        }
    }
};

[benui-dev]

Can confirm this still occurs

[N-Dekker]

Using LLVM 15.04 on Windows, false positive unhandled-self-assignment messages appeared when I adjusted the self-assignment check example from https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-self-assignment.html by adding one more line of code, making it a class template instead:

template <typename MyTemplateParameter> // <== My only adjustment to the example (Niels Dekker, December 2022)
class T {
int* p;

public:
  T(const T &rhs) : p(rhs.p ? new int(*rhs.p) : nullptr) {}
  ~T() { delete p; }

  // ...

  T& operator=(const T &rhs) {
    if(this == &rhs)
      return *this;

    delete p;
    p = new int(*rhs.p);
    return *this;
  }
};

Clang-Tidy says:

operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment,cert-oop54-cpp]

So I guess this specific warning does not yet correctly support class templates, right?

[Mike4Online]

I am also seeing bugprone-unhandled-self-assignment false-positives from Clang Tidy when run against a template class. Here's the beginning of my copy assignment operator for a templated class named Vector:

    Vector<T>& operator=(const Vector<T>& other) // Copy assignment operator
    {
        // Guard self assignment
        if (this == &other)
            return *this; // self-assignment

        if (_Myfirst != nullptr)
            delete[] _Myfirst;

        m_max_size = other.m_max_size;
        _Myfirst = new T[m_max_size];

        m_arraySize = other.m_arraySize;
        for (int32_t i = 0; i < m_arraySize; ++i)
            _Myfirst[i] = other[i];

        return*this;
    }

But I still get a false-positive on the line where this copy assignment operator definition begins. This class is implemented entirely within a header file, as is the norm for a templated class, and there is no separate declaration. Perhaps Clang Tidy is not instantiating the class? Or expecting a declaration distinct from the definition?

I am running clang-tidy.exe version 15.0.7, as provided by Clang Power Tools and LLVM 15.0.7

[Mike4Online]

A note about my previous post, above. My custom Vector class currently lacks an equality operator. That should not matter since the guard within the copy assignment operator is comparing two pointers / addresses to each other:

if (this == &other)

At some point I'll add an equality operator to my class and see if that makes any difference for bugprone-unhandled-self-assignment.