Passing Linked Lists Into Functions

[Sadistictoaster]

Hey

I've set up some linked lists in a program i'm working on , but am having problems using functions with them. The linked lists work fine as long as they stay in the main function : so they're not at fault

struct node
{
int x;
int y;
node *next;
};

is the linked list.

addshot(current,112 , 25); invokes it

void addshot(node *current,int x , int y)
{
current->next = new node;
current = current->next;
current->x = x;
current->next = 0;
}

I've got to admit , I've never really got the hang of pointers ( too much BASIC in my youth ) , but this looks like it should be ok : but am never sure if I need a return or not when dealing with pointers.

I did have a few cracks at going the addshot function with return values , but they'd either not compile , the program would not run , or I would get the same nothing happening as I do with the original function.

Many Thanks

 

[Tony Hoyle]

That routine does nothing except waste some memory. Current is a temporary local variable.

I presume you want current to be returned from the routine... in which case you need to define current as

void addshot(node*& current,int x , int y)

and do the same as before.

btw. since you're using cpp you might as well take advantage of it... the node should self-zero so it's never uninitialised..

struct node
{
node() { x=y=0; next=NULL; }
node(int xx, node yy, node *current) { x=xx; y=yy; next=current; }
int x;
int y;
node *next;
};

void addshot(node *& current,int x , int y)
{
current = new node(current,x,y);
}

Or any variation of similar styles.

Also if you add:

~node() { delete next; }

Deleting the first node (current) will delete the entire list for you.

Depending on how much you're using them investigate std::list too, which takes most of the legwork out. STL is a bit screwy if you're not used to it though..

 

[Parkydr]

It's probably a problem with the value of current when you call addshot. If it starts as 0, current->next isn't going to work. The function won't change the value of current outside the function.

As you're using new, are you using C++? If so look at STL lists - they do it all for you.

 

[jukujin]

struct node
{
int x;
int y;
node *next;
};

...

void addshot(node *current,int x , int y)
{
current->next = new node;
current = current->next;
current->x = x;
current->next = 0;
}

Someone may want to correct me on this, but I think there may be a problem with your parameter list. Typically with a struct, don't you have to declare a variable like so:

struct node *current;

? You may want to define your struct differently, something like:

typedef struct t_node {
   int x, y;
   struct t_node *next;
} node;

I think that would solve a lot of problems. I hate to disagree with Tony, but I don't see why you would have to declare addshot like that, it's kind of redundant. It looks more like you're not derefrencing the address of "current" when you pass it as a parameter. So long as you pass in the address to "current" it should be fine. So you would probably have to call addshot() thusly:

addshot(&current, x, y);

Also, I wouldn't use the parameter value to switch around like that. It's not technically wrong (I don't think), but it's very confusing. I'd do something like the following:

node* addshot(node* pCurrent, int pX, int pY) {
  node* lNewNode;
  lNewNode = new node;
  pCurrent->next = lNewNode;
  lNewNode->x = pX;
  lNewNode->y = pY;
  return lNewNode;
}

It gives you more lines of code, but it's (to me) much easier to understand. (the 'p' and 'l' prefixes aren't Hungarian, it's a tip I picked up from my boss: p stands for parameter, l stands for local. I don't know how I lived without it before I started using it)

That should solve a lot of problems. The thing to remember about pointers is all in the name: all they do is point. Pointers hold nothing but an address in memory. So if you want to pass in a pointer, you have to pass in the address of the memory that you want the pointer to point to, hence the '&' character, to derefence the memory location of the variable 'current'.

My apologies if this isn't quite right, I've been out of ANSI C for while and been doing nothing but classes.

Speaking of which, why not just do a class for it if you're using C++? Then you could skip the scary code Tony pointed out for initialization (as cool as that code is) and just do a constructor.

class node {
   public:
	  int x, y;
	  node(node*, int, int);
	  ~node();
}

Then addshot would be:

{
   node* lNewNode;

   lNewNode = new node(pCurrent, pX, pY);
   pCurrent->next = lNewNode;
}

A bit more overhead with classes, though I doubt it's all that noticeable (then again, SNES9x is coded mostly in assembly, I think, and it shows).

 

[rabidcow]

Someone may want to correct me on this, but I think there may be a problem with your parameter list. Typically with a struct, don't you have to declare a variable like so:

struct node *current;

?

In C, yes. In C++, no. You'll see typedef struct tagx {...} x; quite often in headers that are meant to work with either, but if you're only using C++ there's no point.

I hate to disagree with Tony, but I don't see why you would have to declare addshot like that, it's kind of redundant. It looks more like you're not derefrencing the address of "current" when you pass it as a parameter. So long as you pass in the address to "current" it should be fine. So you would probably have to call addshot() thusly:

addshot(&current, x, y);

You would only have to use &current if addshot were declared as:
void addshot(node ** current,int x , int y);
which also works, if you want to make passing by reference explicit.

Also, I wouldn't use the parameter value to switch around like that. It's not technically wrong (I don't think), but it's very confusing. I'd do something like the following:

node* addshot(node* pCurrent, int pX, int pY) {
  node* lNewNode;
  lNewNode = new node;
  pCurrent->next = lNewNode;
  lNewNode->x = pX;
  lNewNode->y = pY;
  return lNewNode;
}

It gives you more lines of code, but it's (to me) much easier to understand. (the 'p' and 'l' prefixes aren't Hungarian, it's a tip I picked up from my boss: p stands for parameter, l stands for local. I don't know how I lived without it before I started using it)

Then you have to call it with:
current = addshot(current, x, y);
which I personally like because it's more explicit, but it's also more verbose.

Speaking of which, why not just do a class for it if you're using C++? Then you could skip the scary code Tony pointed out for initialization (as cool as that code is) and just do a constructor.

That's exactly the same thing...

I have to agree with Parkydr though. Look at std::list, remember to pass by reference.

 

[jukujin]

I hate to disagree with Tony, but I don't see why you would have to declare addshot like that, it's kind of redundant. It looks more like you're not derefrencing the address of "current" when you pass it as a parameter. So long as you pass in the address to "current" it should be fine. So you would probably have to call addshot() thusly:

addshot(&current, x, y);

You would only have to use &current if addshot were declared as:
void addshot(node ** current,int x , int y);
which also works, if you want to make passing by reference explicit.

Yea, I realized that after I went to bed. I was thinking, actually, of addshot still being declared as (node* current), but calling it with a node type, not a node pointer, e.g.

node current;

current.x = x;
...
addshot(&current, x, y);

Which would make sense with the above, but from the original question, it's pretty obvious all variables referencing a node are pointers, which would make my response incorrect. It was late, cut me some slack.

Then you have to call it with:
current = addshot(current, x, y);
which I personally like because it's more explicit, but it's also more verbose.

He'd still have to do it that unless he actually made the current parameter a double pointer, because keeping it a single pointer wouldn't change the value of the pointer that gets passed as a parameter. It makes more sense to return something if he wants to change the value of the 'current' pointer in the main body of the code, otherwise addshot() stops adding a shot and starts doing multiple things.

I didn't realize the idiosyncracy of structs in C++, but reading O'Reilly's C++ pocket guide, "structs are functionally identical to classes except that the default access level for their members is public, not private." I've always just used classes when I need a class and used structs in the strict ANSI C definition. I can't say I really like the idea of using structs in the "new" way, but if it works ...

Btw, Rabid, this is pika. Who's stalking who now!