Clang-Tidy: Calling a base constructor other than the copy constructor

Question

I'm getting the following message from Clang-Tidy:

Clang-Tidy: Calling a base constructor other than the copy constructor

The code is this:

#include <memory>
#include <set>

using namespace std;

class Node : enable_shared_from_this<Node>
{
public:
    explicit Node(Node *parent = nullptr) : parent(parent) { parent->children.insert(shared_from_this()); }
    Node(const Node &that) : parent(that.parent), children(that.children) {}
    
private:
    Node *parent; // C++14 "weak pointer"
    set<shared_ptr<Node>> children;
};

I can't find documentation for this message from Clang-Tidy anywhere.

What does it mean and why is it giving it?

Some of these clang-tidy messages are just crazy. Turn them off. — lorro, Aug 14 '22 at 20:26
I don't know about clang-tidy message, but you are not allowed to call `shared_from_this()` in constructor. Maybe that's what the message is trying to tell. — Yksisarvinen, Aug 14 '22 at 20:26
Or, actually, I think it's the same as the warning here: https://godbolt.org/z/e4esj5j7e. Your copy constructor doesn't call copy constructor of the base class, it calls it's default constructor. But you are still not allowed to call `shared_from_this()` in constuctors. — Yksisarvinen, Aug 14 '22 at 20:32
Sorry a bit off topic - but if you were using intrusive pointers you could share the smart pointer (or the raw pointer) from the constructor, no problem. — Something Something, Aug 14 '22 at 20:40
Btw. I don't know how you are viewing the output from clang-tidy, but all of its checks have a string identifier which should be part of the message. It can be used to link back to explanation for the check in LLVM/Clang's documentation and can also be used to enable/disable them via the `--checks` option. E.g. I think this should be `bugprone-copy-constructor-init` here, referring to https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-copy-constructor-init.html. — user17732522, Aug 14 '22 at 20:58

score 5 · Answer 1 · answered Aug 14 '22 at 20:47

Clang-Tidy is trying to warn you that your copy constructor fails to call copy constructor of the base class. GCC has similar warning under -Wextra:

<source>: In copy constructor 'Node::Node(const Node&)':
<source>:10:5: warning: base class 'class std::enable_shared_from_this<Node>' should be explicitly initialized in the copy constructor [-Wextra]
   10 |     Node(const Node &that) : parent(that.parent) { if (that.parent) that.parent->children.insert(shared_from_this()); }
      |     ^~~~

You should explicitly use copy constructor in member initializer list:

Node(const Node &that) : enable_shared_from_this(that), parent(that.parent) { if (that.parent) that.parent->children.insert(shared_from_this()); }

There are however other issues with your use of enable_shared_from_this. First, you must always inherit that class publicly:

class Node : public std::enable_shared_from_this<Node>
// ~~~~~~~~~~^

Also, you are not allowed to call shared_from_this() when the owning std::shared_ptr is not yet constructed (notably in class constructor). In C++17 this doing so will throw std::bad_weak_ptr, in earlier standard versions this is Undefined Behaviour.

user17732522 · Answer 2 · 2022-08-14T20:52:47.053

The message is relatively quite clear in my opinion, assuming it points to the copy constructor specifically. It could be improved by referring to the problematic base as well though.

Node inherits enable_shared_from_this<Node>. The copy constructor which you define manually does not mention this base class in the initializer list. Therefore the base class will be default-initialized. Having a base class or member be default initialized in a copy constructor is very likely to not have the intended effect and much more likely to be an oversight. This is why clang-tidy produces the diagnostic.

In this particular situation here there is no problem with enable_shared_from_this<Node> being default-initialized, since its copy constructor is defined to perform the same action as its default constructor. But that is a rare exception.

You can satisfy clang-tidy and avoid relying on this special case, by correctly initializing the base as well:

    Node(const Node &that) : enable_shared_from_this(that), parent(that.parent) { /*...*/}

However, you have more serious problems here. First, std::enable_shared_from_this must be inherited publicly. You are currently inheriting it privately.

Second, you cannot use shared_from_this inside a constructor. When the constructor runs, the object which is being constructed is not yet under control of a std::shared_ptr. As a result calling shared_from_this will simply throw std::bad_weak_ptr (since C++17; undefined behavior before that).

Thanks, this indeed fixed the issue. Regarding the calling `shared_from_this()` from the constructor - so my only viable option is to make the constructor private and use a factory method...? — traveh, Aug 14 '22 at 21:57
@traveh Consider not using the copy constructor in the way you are doing it at all. It violates the typical assumption made of copy constructors that the copy can be used in place of the original without affecting semantics. I am not sure why you chose to implement the copy constructor like this, but it seems to me that it should be a separate member function in the parent. — user17732522, Aug 14 '22 at 22:59
I see your point... I will change it to a `Clone` method in my code and delete the copy constructor in my code... Thanks! — traveh, Aug 14 '22 at 23:12

Clang-Tidy: Calling a base constructor other than the copy constructor

2 Answers2

Linked