What could go wrong if I use a cast to access a protected member?

Question

The following code would not compile because apparently that's how protected members work. (There are questions answering the why on SO, eg. here).

class Parent {
protected:
    int value;
};

class Child : public Parent {
public:
    void set_value_of(Parent* x, int value) {
        x->value = value;
    }
};

int main() {
}

For example, GCC reports

main.cpp: In member function 'void Child::set_value_of(Parent*, int)':
main.cpp:11:16: error: 'int Parent::value' is protected within this context
            x->value = value;
                ^~~~~
main.cpp:5:13: note: declared protected here
        int value;
            ^~~~~

What could possibly go wrong if you cast x to Child? Is it a case of never ever do it or you can, if you know what you're doing?

    void set_value_of(Parent* x, int value) {
        static_cast<Child*>(x)->value = value;
    }

---

---

Edit Unfortunately all suggestions for getting around this abuse so far do not help me in solving my design problem, thus I try to be less generic and show my actualy real-world case.

The main goal is to not rely on friend declaration, as the Node class should not need to be modified (ie. no friend declaration should need to be added), to implement another subclass like Group.

class Node {
protected:
  Node* _parent;
};

class Group : public Node {
protected:
  std::vector<Node*> _nodes;
public:
  void insert(Node* node) {
    if (node->_parent) {
      throw std::runtime_error("node already has a parent");
    }
    _nodes.push_back(node);
    node->_parent = this;
  }
};

This could be solved by add friend class Group to the Node class, but I am looking for a way around it.

Answer 1

Note that, considering your actual code, I would suggest refactoring if Group isn't intended to be a special type of Node. Please see my note at the bottom of this answer.

---

If you can modify Parent, one way to do this would be to provide a protected member function as a helper; considering that set_value_of() takes an instance and a value, it should probably be a static one.

class Parent {
protected:
    int value;

    // Like this.
    static void set_value_of(Parent* x, int value) { x->value = value; }
};

class Child : public Parent {
public:
    // Provide set_value_of() to the world.
    using Parent::set_value_of;

    // Alternatively...
    //static void set_value_of(Parent* x, int value) {
    //    Parent::set_value_of(x, value);
    //}


    // For example usage.
    int get_value() const { return value; }
};

int main() {
    Child c1, c2;

    c1.set_value_of(&c2, 33);
    c2.set_value_of(&c1, 42);
    Child::set_value_of(&c2, 3);

    std::cout << c1.get_value() << ", " << c2.get_value() << '\n';
}

With this, the output will be 42, 3, as expected.

---

Considering your actual code, this can still be implemented. The issue is that Node doesn't provide a way for outside influences to safely modify it, which by extension means that it doesn't provide a way for Group to safely modify it. (While Group can indeed try to cast Node* to Group*, that could very well go wrong, and it would likely confuse anyone performing maintenance in the future.) Therefore, if Node is intended to be a base class, and leave actually managing the various nodes to subclasses, it should provide a way for them to tell it what to do.

Additionally, considering what you're trying to do, I would suggest providing another helper function, has_parent(). This allows subclasses to easily check whether a given node has a parent, without providing access to the outside world.

class Node {
protected:
  Node* _parent;

  static void set_parent(Node* n, Node* parent) {
    n->_parent = parent;
  }

  static bool has_parent(Node* n) {
    return n->_parent != nullptr;
  }
};

class Group : public Node {
protected:
  std::vector<Node*> _nodes;
public:
  void insert(Node* node) {
    if (has_parent(node)) {
      throw std::runtime_error("node already has a parent");
    }
    _nodes.push_back(node);
    set_parent(node, this);
  }
};

---

Note, however, that unless Group is intended to be a special node which manages other nodes, then I don't recommend making it a subclass of Node; rather, if there's no is-a relationship, Node should declare Group as a friend. Contrary to somewhat-popular belief, this doesn't break encapsulation; rather, it increases encapsulation, because it enables Group to manage Node without providing a back door that anyone else can use to get into Node, too (any code can use accessors & mutators, and any class that has Node as a parent can access protected helpers, and Node would be powerless to stop them; conversely, a friend declaration lets Node allow Group to access it, while denying access to the rest of the universe).

Conversely, if Group really is a type of Node, then Node should have a virtual destructor, so it can be used polymorphically. After all, there's no way for Group to know which of its nodes are other Groups.

int main() {
    Group g;

    g.insert(new Node);  // Valid.
    g.insert(new Group); // Valid.  Group is just a Node in a fancy suit, right?
}

Answer 2

You can make it cleaner.

void set_value_of(Parent* x, int value) {
    Child* ch = dynamic_cast<Child*>(x);
    if ( ch != nullptr )
    {
       ch->value = value;
    }
}

It is robust and stays within the rules of the language.

---

Based on the real code ...

Node, as posted, is a rather poorly designed class. It really should make parent_ a public member variable or provides functions to get and set the member variable.

If you have option, change it to something like:

class Node {
  public:
     Node(Node* p = nullptr) : _parent(p) {}
     Node* getParent() const { return _parent; }
     void setParent(Node* p) { _parent = p; }

  private:
    Node* _parent;
};

Answer 3

What could possibly go wrong if you cast x to Child? Is it a case of never ever do it or you can, if you know what you're doing?

Lots of things, which is why you should be using a dynamic_cast, not a static_cast.

void set_value_of(Parent* x, int value) {
    Child * child = dynamic_cast<Child*>(x);
    if(child) child->value = value;
}

dynamic_cast will ensure that the cast will return nullptr if the object being pointed to is not, in fact, a Child object.

EDIT: based the comments taking place, it sounds like inheritance is the wrong design for these objects. If Child should have direct agency over the Parent objects it refers to, then Child should be made a friend of Parent, and should not derive from Parent at all. What you're doing is inherently error-prone and represents confusing design.

class Parent {
    int value;
    friend class Child;
};

class Child {
public:
    void set_value_of(Parent* x, int value) {
        //Is now valid because Child is a friend of Parent
        x->value = value;
    }
};