C++ - clang tidy complain about rule of X?

Question

This code raises a warning in clang tidy:

Class 'Locker' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operatorclang-tidy(cppcoreguidelines-special-member-functions)

This is the updated struct, according to the comments:

struct Locker
{
    std::binary_semaphore *sem = nullptr;

    // ----------------------------------
    // Methods
    // ----------------------------------
    auto try_lock(std::binary_semaphore &sem_, u32 time_in_seconds = 1) -> bool;

    auto manual_release() -> void;

    // ----------------------------------
    // Deleted
    // ----------------------------------

    Locker(Locker &) = delete;

    Locker(Locker &&) = delete;

    Locker(std::binary_semaphore &&sem_) noexcept = delete;

    Locker(std::binary_semaphore &sem_) noexcept = delete;

    Locker(std::binary_semaphore *sem_) noexcept = delete;

    Locker() noexcept = default;

    auto operator=(std::binary_semaphore &sem_) noexcept -> Locker & = delete;

    auto operator=(std::binary_semaphore &&sem_) noexcept -> Locker & = delete;

    auto operator=(std::binary_semaphore *sem_) noexcept -> Locker & = delete;

    // ----------------------------------
    // Destructor
    // ----------------------------------
    ~Locker()
    {

        manual_release();
    }
};

I don't want any constructors but I want a specific destructor. we have method to try lock and the destructor just release the lock cleanly making sure everything is ok.

Please note that GCC 12.2 with

-Wall -Wextra -pedantic -pedantic-errors -Werror -Wuninitialized -Wtrivial-auto-var-init -Wshadow -Wnormalized -Wno-error=comment

doesn't even bother.

How to suppress that warning?

Thanks!

Edit:

Below is the godbolt link to replicate the situation, may someone correct my code and share the link here? Link to clang-tidy struct issue

rather than surpress I'd define them. Maybe `= default`ing them is already sufficient, did you try that? — 463035818_is_not_an_ai, Feb 17 '23 at 08:50
@463035818_is_not_a_number : the default raises: Only special member functions and comparison operators may be defaultedclang(default_special_members) — LeXav, Feb 17 '23 at 08:53
Constructor and operator *by value* are obsolete, these are covered already by const reference – if implementing both you'll run into ambiguities. — Aconcagua, Feb 17 '23 at 08:55
@Aconcagua: i improved the code according to your remark; I will reflect that in the post — LeXav, Feb 17 '23 at 08:56
then you must have made a typo. The warning does complain only about those special members that can be defaulted. — 463035818_is_not_an_ai, Feb 17 '23 at 08:58
R-value reference constructor likely is not meaningful even if you *do* implement the others, apparently `Locker` doesn't take ownership of the object anyway. — Aconcagua, Feb 17 '23 at 08:58
@Aconcague I am refering to my suggestion to `=default` the special members rather than surpressing the warning, not to the constructors already present in the code — 463035818_is_not_an_ai, Feb 17 '23 at 09:00
Question is now how you would want to handle the copying/assignment. Shall `Locker` itself be copiable? Currently you allow implicit move and copy constructors, though it is not clear if that's meaningful considering the semaphore held and the destructor likely going to release it while constructor acquires it. — Aconcagua, Feb 17 '23 at 09:01
@463035818_is_not_a_number Ah, I see. Reply of question author was a red herring then ;) — Aconcagua, Feb 17 '23 at 09:02
I think the crucial point here is that the rule of 5 is not only important to clang tidy. It is important to human readers of your code. If you implement one then you should provide the others too. If you don't then probably your code has a bug, and even if not it is confusing, because one expects the rule of 5. The rule is so important that I'd rather add some code that does nothing , like eg a redundant `special_member() = delete;` or `speical_member() = default;`. If you do stick to the rule of 5 then also the clang tidy warning will go away — 463035818_is_not_an_ai, Feb 17 '23 at 09:05
@Aconcagua: Locker is not copyable, it just take the pointer to a semaphore via a try_lock method (not shown here). then when the scope reaches the end, it automatically release the semaphore. Nothing more — LeXav, Feb 17 '23 at 09:06
if it is not copyable, why is there no `Locker(const Locker&) = delete;` ? — 463035818_is_not_an_ai, Feb 17 '23 at 09:07
Pretty much what I assumed from the naming, the fact of holding a semaphore and the output of the destructor ;) Then what you actually need is `Locker(Locker const&) = delete` to get rid of the defaulted copy constructor. That alone will already delete the move constructor as well, though it might be meaningful to indeed be able to *move* objects of that type – then you'd define such one – making sure the pointer of the object being moved is set back to `nullptr`. — Aconcagua, Feb 17 '23 at 09:09
Adding Locker(const Locker &) = delete; and Locker(const Locker &&) = delete; the warning is still here — LeXav, Feb 17 '23 at 09:09
`const` r-value references are pretty meaningless apart from some few very specific special cases – the default move constructor accepts non-const `Locker&&` – though on deleting it anyway it doesn't really matter. — Aconcagua, Feb 17 '23 at 09:11
You still need to do the same for the assignment operator: `Locker& operator=(Locker const&) = delete`. — Aconcagua, Feb 17 '23 at 09:14
And you do not need to delete any non-standard constructors – if you simply do not define them they won't exist. So all that you *really* need – apart from deleted standard copy/move constructors/assignments (those accepting some kind of reference to `Locker`) is a constructor accepting a reference or pointer (depending on what you consider more meaningful, I personally tend to reference) while you simply can remove all others accepting any kind of const semaphore reference or pointer. — Aconcagua, Feb 17 '23 at 09:17
I removed all the const, but still having the warning. Let me update in my post — LeXav, Feb 17 '23 at 09:18
Summary: All you really *need* is: `Locker(std::binary_semaphore& s) : sem(&s) { } Locker(Locker const&) = delete; Locker& operator=(Locker const&) = delete` – deleting move constructor and assignment is optional, they will get deleted anyway (by either defining, defaulting or deleting a copy variant of), but of course you *can* still add them for being more explicit... — Aconcagua, Feb 17 '23 at 09:20
Any combination will still lead to the warning. it is driving me crazy — LeXav, Feb 17 '23 at 09:21
You removed `const` from the copy constructor/assignment as well – note: l-value reference -> const -> copy, r-value reference -> non-const -> move! — Aconcagua, Feb 17 '23 at 09:32
To all, I added a godbolt compiler link, all the option with clang tidy and so on, may someone correct my code and share the link? — LeXav, Feb 17 '23 at 09:39
Demonstration of a [*movable* `Locker`](https://godbolt.org/z/7T5r1zr3n) – if it appears meaningful to you. Possibly not really relevant here but you might remember the pattern for other purposes in the future ;) — Aconcagua, Feb 17 '23 at 10:10
oh cool, i didnt know that godbolt can clangtidy, that makes things a lot easier :) — 463035818_is_not_an_ai, Feb 17 '23 at 11:13

463035818_is_not_an_ai · Accepted Answer · 2023-02-20T09:00:46.417

1

Mostly by help from Aconcagua and n.m. we found out what your misconception was.

This has the wrong signature:

Locker( Locker &) = delete;

Copy constructor takes a const Locker&. And you do not delete the assignment. Those are the five:

Locker(const Locker &) = delete;
Locker(Locker &&) = delete;
Locker& operator=(const Locker&) = delete;
Locker& operator=(Locker&&) = delete;
~Locker() { 
    std::cout << "";
 }

If you explicitly declare them all, the warning should be gone: https://godbolt.org/z/eP8TrE9b1

edited Feb 20 '23 at 09:00

answered Feb 17 '23 at 11:17

463035818_is_not_an_ai

109,796
11
89
185

Thanks a lot! Same for Aconcagua but i can't upvote – LeXav Feb 17 '23 at 11:55
@LeXav I added some credits to the answer. I was basically just writing things from comments up in an answer – 463035818_is_not_an_ai Feb 17 '23 at 11:57
Thank you, that's a nice move from you – LeXav Feb 17 '23 at 14:19

C++ - clang tidy complain about rule of X?

Edit:

1 Answers1