0

I have code which appears to work even though I have a bad feeling about it.

My class Node has private members:

// Node properties
std::string m_keySymbol = "";
float m_nodeWeight = 0.0f;
std::string m_displaySymbol = "";

// Ultimately will be either 0x00 or 0x01.  This is a starting and testable value.
//std::uint8_t m_branchBitValue = 0xFF;

// Node relationships
Node* mp_parentNode = nullptr;

//std::pair<Node*, Node*> mp_childNodes = { nullptr, nullptr };
NodeBranches m_nodeBranches;
bool m_nodeHasBranches = false;

and a method with the definition of:

Node::Node(Node& left, Node& right) {

    m_keySymbol = left.m_keySymbol + right.m_keySymbol;
    m_nodeWeight = left.m_nodeWeight + right.m_nodeWeight;
    m_displaySymbol = left.m_displaySymbol + right.m_displaySymbol;

    m_nodeHasBranches = true;
    m_nodeBranches.left.first = const_cast<Node*>(&left);
    m_nodeBranches.left.second = 0x00;
    m_nodeBranches.right.first = const_cast<Node*>(&right);
    m_nodeBranches.right.second = 0x01;

    left.mp_parentNode = this;
    right.mp_parentNode = this;
}

My specific concern is with the last two lines of Node::Node(Node& left, Node& right) and specifically where I have used this as a rvalue.

In main() the following code works:

    // Start the combining process.
    Node cb(nodesVector[0], nodesVector[1]);

    std::cout << "Address of cb: " << std::hex << &cb << std::endl;
    std::cout << "Parent node of nodesVector[0]: " << std::hex << nodesVector[0].GetParentNode() << std::endl;

that is, the address of cb matches that returned by nodesVector[0].GetParentNode(), which returns mp_parentNode.

I have looked and cannot find an example where this is used as an rvalue even though this is defined to be prvalue expression which has the properties of an rvalue.

Am I missing something?

user34299
  • 377
  • 2
  • 11
  • `const_cast(&left)` is problematic for two reasons: The first is that `&left` is not a pointer to a constant so there's no need for a `const_cast`. The second is if the life-time of the object referenced by `left` ends, and it's destructed. Trees are usually handled with dynamically allocated node objects, not by using the address-of operator `&`. – Some programmer dude Apr 24 '20 at 14:49
  • The only way this is safe is if you can guarantee with 100% certainty that the object in `nodesVector` will outlive the object `cb`. – Some programmer dude Apr 24 '20 at 14:52
  • Thanks. I had not considered that. It does work for the reason you gave, viz., that `nodesVector[0]` and `nodesVector[1]` do live as long as does `cb`. – user34299 Apr 24 '20 at 15:03

1 Answers1

0

My specific concern is with ... specifically where I have used this as a rvalue.

There is nothing to be concerned about that. As you said, this is a prvalue expression. Prvalues are rvalues and they can only be used as an rvalue.

What you should be concerned about is storing pointers to reference arguments. It is very easy to accidentally use the constructor with objects whose lifetime ends before the constructed node. I recommend using pointer arguments instead so that it is a bit clearer to the caller that a pointer to the object will be stored. Also document it carefully.

Also, the const_casts are redundant.

eerorika
  • 232,697
  • 12
  • 197
  • 326