Pointer help (again)....

Hi,

I have a god structure as such:

Code:

---

struct God {
        God(string n, string m, string v = "", string w = "")
                :name(n), mythology(m), vehicle(v), weapon(w) {}


private:
        string name;
        string mythology;
        string vehicle;
        string weapon;
};

---

And a Link structure which I am using pointers to created a linked list. The Link structure is:

Code:

---

class Link {
public:
        God god1;
        Link(God& g, Link* p = 0, Link* s = 0)
                : god1(g), prev(p), succ(s) {}

        Link* insert(Link* n);
        Link* add(Link*n);
        Link* erase(Link* p);
        Link* find(const string& s);
        const Link* find(const string& s) const;

        Link* advance(int n) const;

        Link* next() const {return succ;}
        Link* previous() const {return prev;}

       
private:
        Link* prev;
        Link* succ;
       
};

---

When I try to create a Link object as such:

Code:

---

Link* norse_gods = new Link(("Odin", "Norse", "Eight legged horse"), 0, 0);

---

I get an error saying: error C2664: 'Link::Link(God &,Link *,Link *)' : cannot convert parameter 1 from 'const char *' to 'God &'

As far as I can see I havent't defined any consts that would cause this?

Any advice please.

Thanks,

Darren.

You are trying to pass a temporary to the constructor which doesn't accept temporaries.
Is the Link going to modify the object? If not, then make it take const God&. If it's going to modify the object, you have to construct it first:

God blah(...);
Link(blah, ...);

But why are you creating a linked list in the first place? Why not use std::list?
Furthermore, you should be using smart pointers, eg:

std::shared_ptr<Link> norse_gods(God("Odin", "Norse", "Eight legged horse"), 0, 0);

Also, your names are bloody poor:
string n, string m, string v = "", string w = ""
What is n? What is m? What is v? And what the heck is w!?!?!?!?!?!!!????!

Quote:

---

Originally Posted by Elysia

You are trying to pass a temporary to the constructor which doesn't accept temporaries.
Is the Link going to modify the object? If not, then make it take const God&. If it's going to modify the object, you have to construct it first:

God blah(...);
Link(blah, ...);

But why are you creating a linked list in the first place? Why not use std::list?
Furthermore, you should be using smart pointers, eg:

std::shared_ptr<Link> norse_gods(God("Odin", "Norse", "Eight legged horse"), 0, 0);

Also, your names are bloody poor:
string n, string m, string v = "", string w = ""
What is n? What is m? What is v? And what the heck is w!?!?!?!?!?!!!????!

---

Okay, I understand about consts and simply forgot to include that in this. Regarding the construction, are you saying that I cannot construct an object of type1 inside the constructor of type2 and that I can only use constructed objects as a parameter?

The reason I am doing it this way, is that the book is using this to demonstrate the use of pointers, we have not yet reached the stage of smart pointers.

The names I have used are simply temporary objects to initialise the member data of the class. Should I also be giving them meaningful names as well?

Thanks.

What I'm saying is that when you do something like...

MyFunc( God(...) )

...you are creating a temporary God object (because it's not stored anywhere). The standard says that you cannot store a temporary (rvalue) in a non-const reference (which can only bind lvalues). In other words, you cannot modify a temporary (OK, there are exceptions).

Therefore, this works:

const God& tmp = God(...);

And this doesn't work:

God& tmp = God(...);

However, this does work:

God god(...);
God& tmp = god;

Because god is not a temporary.

And yes, all names should be meaningful. When you call that function later, you're going to wonder what the hell those n, v and w mean.

Oh and this syntax:

("Odin", "Norse", "Eight legged horse")

...just isn't valid. It will evaluate to a single const char* (as the compiler complains) because just basically just invoking the comma operator (read up on it if you will).
You need to specify the type you're creating and passing in its arguments, as in

God("Odin", "Norse", "Eight legged horse")

Now you're creating a temporary God object.
(Should have mentioned that earlier.)

Quote:

---

Originally Posted by Elysia

What I'm saying is that when you do something like...

MyFunc( God(...) )

...you are creating a temporary God object (because it's not stored anywhere). The standard says that you cannot store a temporary (rvalue) in a non-const reference (which can only bind lvalues). In other words, you cannot modify a temporary (OK, there are exceptions).

Therefore, this works:

const God& tmp = God(...);

And this doesn't work:

God& tmp = God(...);

However, this does work:

God god(...);
God& tmp = god;

Because god is not a temporary.

And yes, all names should be meaningful. When you call that function later, you're going to wonder what the hell those n, v and w mean.

Oh and this syntax:

("Odin", "Norse", "Eight legged horse")

...just isn't valid. It will evaluate to a single const char* (as the compiler complains) because just basically just invoking the comma operator (read up on it if you will).
You need to specify the type you're creating and passing in its arguments, as in

God("Odin", "Norse", "Eight legged horse")

Now you're creating a temporary God object.
(Should have mentioned that earlier.)

---

Thanks for the clear response. I understand that now.

I would agree that you should use the STL, unless it is an exercise in writing a linked list (something I was asked to do in a job interview last week BTW).

Out of interest, in the container class, what are the "Insert" and "Add" methods returning. They seem to be returning pointers to links, but what object are they pointing to, as the new link is being passed in as the parameter. Returning pointers from the "Find" or "Erase" functions is fine, but I'm not sure what Insert and Add are doing.

Quote:

---

Originally Posted by CodeMonkey62

I would agree that you should use the STL, unless it is an exercise in writing a linked list (something I was asked to do in a job interview last week BTW).

Out of interest, in the container class, what are the "Insert" and "Add" methods returning. They seem to be returning pointers to links, but what object are they pointing to, as the new link is being passed in as the parameter. Returning pointers from the "Find" or "Erase" functions is fine, but I'm not sure what Insert and Add are doing.

---

Yeah, it was an exercise in building a linked list. The code for Insert is:

Code:

---



Link* Link::insert(Link *n)
{
        if(n==0) return this;
        if(this==0) return n;
        n->succ = this;
        if(prev) prev->succ = n;
        n->prev = prev;
        prev = n;
        return n;
}

---

I haven't put the add code in yet.

Don't use 0. Use nullptr.
this == nullptr should NEVER be true. If so, you've done something wrong. Use an assert, eg assert(this != nullptr);
Also, though your return is valid, usually insert doesn't really return anything, because it's pointless.

Quote:

---

Originally Posted by Elysia

Also, though your return is valid, usually insert doesn't really return anything, because it's pointless.

---

That was my point really. The code looks reasonable (I'm not sure that I'd test for "this" being null personally, it should never be). It could do with some better formatting too.

Quote:

---

Originally Posted by CodeMonkey62

...(I'm not sure that I'd test for "this" being null personally, it should never be)...

---

Hence the assert. Asserts are good for finding program states that should never be.

If the condition should never be, why on R'lyeh are you advocating the use of a check that is naturally not done in release mode?

Soma

o_O
Who suggested such a thing?

Quote:

---

assert(this != nullptr);

---

I assumed you were with the reference to `assert'.

I'm sorry if you had intended some other form of assert that sticks around even in a release build.

Soma