-1

I'm relatively new to c++. I have a binaryTree class, that contains Nodes. Inside binaryTree, I want the "addleft" method to take in a node (and the potential subtree attached below it) and just "point" the root.left pointer to the "inputnode", without deep copying the node and its entire subtree that could come with it during the process.

struct Node {
    int val;
    Node* left;
    Node* right;
    Node(int val) : val(val), lc(nullptr), rc(nullptr) {}
    Node() : val(0),lc(nullptr), rc(nullptr) {}

}; 


class binaryTree {
    public:
        Node root;
        int size;
        binaryTree(const Node& node) : root(node), size(1) {} 
        addleft(const Node& inputnode) {root.left=&inputnode;} //the problem is here

};

I'm under the impression that its common for functions to use take in parameters using "const type& variable" (Correct me if I'm wrong), so I use it here in "addleft" method. But now its bad because I can't assign "inputnode" to "root.left" since constant node object/reference can't be assigned to nonconstant pointer of root.

One option is to deep copy the "inputnode" and assign its address to root.left, but I'm trying to avoid that.

Is there any way for me to implement what I want the "addleft" method to do without changing the "const Node& input", or should I just not use "const Node& input" here? If I choose to abandon the "const Node& input" parameter, is it good conventions for me to write two "addleft" methods, one taking in "Node& input", the other "Node&& input", so I can deal with both temporary variables and nontemporary variables?

Edit: I'm told that using "Node& input" and "Node&& input" like below would lead to dangling pointers.

        void addleft(Node& node) {
            root.lc=&node;
        }

        void addleft(Node&& node) {
            root.lc=&node;
            
        }

However when I tested it, it still seems to run normally, with root.lc still pointing towards a Node of value 12 after the program ends. What am I missing?

int main () {

    binaryTree a(Node(15));

    a.addleft(Node(12));

    cout<<a.root.lc->val<<endl; 

    cout<<"finished"<<endl;

}
JohnZ
  • 87
  • 8
  • 2
    `const` is a promise that you won't modify the object. If you then store it as a non-`const` pointer you are breaking that promise, because it will be possible to modify the object through that pointer. "_the other "Node&& input, so I can deal with both temporary variables and nontemporary variables?_": You explicitly shouldn't want this. A temporary object will be destroyed immediately. You should never store a pointer to a temporary object. The pointer will become dangling shortly after the function exits. – user17732522 Aug 26 '23 at 10:02
  • @user17732522, I see. So what should I do in this case? Should I use some other type of parameter? – JohnZ Aug 26 '23 at 10:04
  • 2
    Remove the `const`. However, you probably need to rethink the whole design. Why is it the users responsibility to hand the `binaryTree` a node? The node should be an implementation detail of the tree. It should allocate/create and manage them itself. – user17732522 Aug 26 '23 at 10:05
  • @user17732522, but I'm concerned that dropping the constant and just doing Node& input would basically bar myself from putting in temporary variables into the function, like doing myTree.addleft(Node(5)); – JohnZ Aug 26 '23 at 10:16
  • @user17732522 as for the design, I agree that its bad to ask users to hand in a node, but I'm just trying to understand the mechanics of how c++ parameter passing works, and binaryTree is just used for the sake of asking the question. – JohnZ Aug 26 '23 at 10:18
  • 1
    It would be wrong to do that. The `Node(5)` is destroyed immediately after that point. You shouldn't store a pointer to it. Trying to access it later will cause undefined bheavior. You need to store each `Node` externally somewhere. That's why I am saying that the whole approach is flawed. The tree class should itself use `new`/`delete` (maybe `std::unique_ptr`/`std::make_unique` with many caveats) or a generic allocator to create and destroy `Node` objects. The user should only provide values to store in the node. (That's assuming you don't intent to construct an intrusive data structure.) – user17732522 Aug 26 '23 at 10:20
  • @user17732522 I see, it seems that the problem wouldn't exist if the example class is better designed. Thanks a lot for your help! – JohnZ Aug 26 '23 at 10:35
  • @user17732522 Regarding your first comment on how using Node&& would lead to dangling pointers, I edited my post and showed how during testing, the dangling pointer doens't seem to be a problem when running the proposed function. I think I'm missing something here though. – JohnZ Aug 26 '23 at 10:48
  • 1
    `a.root.lc` is a dangling pointer in `a.root.lc->val` and so that evaluation has _undefined behavior_. Undefined behavior means that _anything_ can happen. You have no guarantees on how the program will behave. Seemingly behaving as you expect is one possibility, but so is any other. You can't test whether a program has undefined behavior or is valid in C++ by trial. Once you slightly modify your program or try a different compiler or even just run it again, etc., it may fail with an error or produce wrong output or do anything else. – user17732522 Aug 26 '23 at 11:09
  • 1
    yes it is recommended to pass parameters as `const&` when possible. However you seem to miss that pointers can be passed by const reference as well. If you want to pass a pointer then pass a pointer: `void addleft(const Node*& node)` but then passing a pointer as reference is pointless, because passing a pointer is as expesnive as passing a reference, and you can simply pass the pointer by value `void addleft(Node* node)` – 463035818_is_not_an_ai Aug 26 '23 at 11:15
  • 1
    That `BinaryTree` class is weird. It manages the lifecycle of the root node and the root node alone. Anything else you'd need to dynamically allocate which would also be a bit weird, since the root would be allocated as part of the memory of `BinaryTree`, but the other ones would be allocated via `new`. My recommendation would be to deprive the user of any direct access to the node objects, but simply provide a wrapper the user could use to retrieve the node value and possibly navigate through the tree (similar to container iterators in the standard library). – fabian Aug 26 '23 at 11:25
  • 1
    Presumably, the function that calls `some_binary_tree.addLeft(some_node)` has to obtain `some_node` from somewhere - it will either have created the node itself, or been passed that node somehow. Instead of requiring the *caller* to obtain (or create) the node, change your function to accept the *data* used to create the node. Then your `addLeft()` function can safely construct the node (no "deep copy") and link it correctly with other nodes that are managed by an instance of `binaryTree`. – Peter Aug 26 '23 at 11:41
  • 1
    Perhaps the `root` of a `binaryTree` should be a pointer to the root node, rather than a node itself. That way, `root`, `left`, and `right` would all share a common type: `Node*`. – tbxfreeware Aug 26 '23 at 12:53
  • 1
    `Node root;` is an error. Don't do that. Use `Node* root;`. Otherwise your tree cannot be empty, which just makes your life tremendously more difficult. – n. m. could be an AI Aug 26 '23 at 15:45
  • @fabian, can you elaborate on what you mean by "Anything else you'd need to dynamically allocate"? When I add a new node to a tree, I'm planning to just traverse through the tree, and simply do something like "finalnode.left = givenInputNode"; I'm not sure on what you're referring to when you say that certain things need to be dynamically allocated. – JohnZ Aug 27 '23 at 01:07
  • 1
    You've got an overload of `addleft` that takes an rvalue reference. For using that overload you need to either pass an xvalue that will go out of scope when the full full expression containing the function call completes leaving you with a dangling reference, or you cast a reference to an rvalue reference using `std::move` in which case you could just remove the `std::move` and use the other overload, so you will end up with a dangling `left` pointer unless you somehow allocate a new `Node` object which would need to be stored in dynamically allocated memory... – fabian Aug 27 '23 at 07:01
  • Aha, that makes lots of sense now. Thanks a lot @fabian! – JohnZ Aug 27 '23 at 07:31

0 Answers0