Like Tree1Likes

linked list issues

This is a discussion on linked list issues within the C++ Programming forums, part of the General Programming Boards category; OK so I seem to have a memory leak here or something I fixed the issue where head wasn't being ...

  1. #1
    Registered User
    Join Date
    Oct 2010
    Posts
    7

    linked list issues

    OK so I seem to have a memory leak here or something I fixed the issue where head wasn't being written to but at the end of the Append function with this data head CXX0017: Error: symbol "" not found
    UID CXX0030: Error: expression cannot be evaluated

    head is being written to but seems to reset at the end of the function with not NULL pointers.


    CPP file
    Code:
    #include "stdafx.h" 
    
    
    
    cList::cList(){
    	UID = 0;
    	head = NULL;
    }
    
    
    void cList::Render(){
    	cNode *tmp = head;
    
    	if( tmp == NULL){
    		return;
    	}
    
    	if( tmp->Next() == NULL) {
    		tmp->RenderData();
    	} else {
    		//render data
    
    		do{
    			tmp->RenderData();
    
    			tmp = tmp->Next();
    		}while(tmp != NULL);
    	}
    
    }
    
    void cList::Update(){
    	cNode *tmp = head;
    
    	if( tmp == NULL){
    		return;
    	}
    
    	if( tmp->Next() == NULL) {
    		tmp->RenderData();
    	} else {
    		//render data
    
    		do{
    			tmp->UpdateData();
    
    			tmp = tmp->Next();
    		}
    
    		while(tmp != NULL);
    	}
    
    }
    
    // creating now nodes
     void cList::Append(cStar data){
    
    	 cNode* newNode = new cNode();
    	 UID++;
    	 newNode->SetData(data, UID);
    	 newNode->SetNext(NULL);
    
    	 //head->SetData(data,UID);
    
    
    	 cNode *tmp = head;
    	 if(head == NULL){
    		 head = newNode;
    	 } else if( head->Next() == NULL){
    		 head->SetNext(newNode);
    	 } else{
    		 while(tmp->Next() != NULL){
    			 tmp = tmp->Next();
    		 }
    	 }
    
    	 
    
     }
    
     void cList::Delete(cStar data){
    	 cNode *tmp = head;
    
    	 if ( tmp == NULL){
    		 return;
    	 }
    	 if( tmp->Next() == NULL){
    		 delete tmp;
    
    		 head = NULL;
    
    	 } else {
    		 cNode *prev;
    
    		 do{
    
    			 if(tmp->Data().GetUID() == data.GetUID() ) break;
    			 prev = tmp;
    
    			 tmp = tmp->Next();
    		 } while (tmp != NULL );
    
    		 prev->SetNext(tmp->Next());
    
    		 delete tmp;
    
    	 }
     }
    this is the header
    Code:
    #pragma once
    //start of node
    class cNode{
    
    	cStar data;
    
    	cNode* next;
    
    public:
    	cNode() { next = NULL;};
    
    	void SetData(cStar aData, int iData) {
    		data.SetUID(iData);
    		//data.SetData(aData);
    		data = aData; // just testing something
    	};
    	void SetNext(cNode* aNext){
    		next = aNext;
    	};
    	void RenderData(){
    		data.Render();
    	}
    	void UpdateData(){
    		data.Update();
    	}
    	cStar Data(){
    		return data;
    	};
    	cNode* Next(){
    		return next;
    	};
    };
    
    //end of node
    
    class cList {
    	int UID;
    	cNode *head;
    
    public:
    	cList();
    
    	void Render();
    	void Update();
    
    	void Append(cStar data);
    
    	void Delete(cStar data);
    
    };
    any help would be awesome
    Last edited by BlizAce; 01-26-2014 at 09:54 AM.

  2. #2
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,825
    The final else of your append function doesn't actually do anything.

    Your update function calls renderdata as a special case.

    Most of the code is too complicated.

    This is all you need in all your list traversals. You don't need special cases for 0, 1, 2 nodes.
    Code:
    cNode *tmp = head;
    while ( tmp != NULL ) {
      // do something
      tmp = tmp->Next();
    }
    Apart from the special case of insert / delete at the head of the list (when you need to update head itself), it should be very simple.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    ok yeah that code is a lot better. now im just having and issue where head is still not being set.
    Code:
     void cList::Append(cStar data){
    
    	 cNode* newNode = new cNode();
    	 UID++;
    	 newNode->SetData(data, UID);
    	 newNode->SetNext(NULL);
    
    	 //head->SetData(data,UID);
    
    
    	 cNode *tmp = head;
    	 while ( tmp != NULL ) {
    		tmp = tmp->Next();
    			}
    	 tmp = newNode;
    
     }

  4. #4
    Registered User
    Join Date
    Jun 2013
    Posts
    56
    You declare a pointer to head, but you never declare head to point to an actual node

    After setting newNode you should have head point to that to start your list. Doing that means head will point to an actual node, then you can begin adding to the list etc. You do want to check that head is null first to insure you are starting the list on in the middle of the list creation.

    *Edit* Hmm after re-reading the code update you do actually set it to a node at the end. Nevermind me

    The issue I am guessing lies in the functions you are not yet showing us.
    Last edited by Ewiv; 01-26-2014 at 10:45 AM.

  5. #5
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    here is my other code. I cannot fix this and i have spent a lot of time on it. sorry about my other code... its a little... scrubby

    cStar cpp file
    Code:
    #include "stdafx.h"
    
    cStar::cStar(){
    	srand (time(NULL));
    
    
    	image = al_load_bitmap("./resources/images/star.png");
    	if(!image){
    	   fprintf(stderr, "failed to create bouncer bitmap!\n");
       }
    	fVel_X = 0.0;
    	fVel_Y = rand() % 4 + 1;
    	fPos_X = rand() % 800 + 1;
    	fPos_Y = 0.0;
    
    }
    
    bool cStar::init_Star(bool arg){
    	srand (time(NULL));
    
    
    	image = al_load_bitmap("resource/images/star.png");
    	if(!image){
    	   fprintf(stderr, "failed to create bouncer bitmap!\n");
       }
    	if(arg){
    		fVel_X = rand() % 4 + 1;
    		fVel_Y = 0.0;
    		bMenu = true;
    	} else {
    		//add random code
    		bMenu = false;
    	}
    
    	return true;
    }
    
    void cStar::Update(){
    	fPos_X += fVel_X;
    	fPos_Y += fVel_Y;
    	fprintf(stderr, "update\n");
    }
    
    void cStar::Render(){
    	//al_draw_bitmap(image, 0, 0,0);
    	al_draw_bitmap(image, fPos_X, fPos_Y,0);
    }
    
    void cStar::SetData(cStar sData){
    	iSize_X = sData.iSize_X;
    	iSize_Y = sData.iSize_Y;
    	fVel_X = sData.fVel_X;
    	fVel_Y = sData.fVel_Y;
    	fPos_X = sData.fPos_X;
    	fPos_Y = sData.fPos_Y;
    	bMenu = sData.bMenu;
    }
    cStar .h file
    Code:
    #pragma once
    
    class cStar: public cObject {
    public:
    	
    
    	//data
    	int iSize_X, iSize_Y;
    	float fVel_X, fVel_Y, fPos_X, fPos_Y;
    	bool bMenu;
    
    	//functions
    	bool init_Star(bool arg);
    	void Update();
    	void Render();
    	void SetData(cStar sData);
    	cStar();
    	~cStar(){
    		al_destroy_bitmap(image);
    	}
    private:
    	ALLEGRO_BITMAP *image;
    
    };
    ok time for the main function that calls it... sorry this code is pretty sloppy
    Code:
    #include "stdafx.h"
    
    
    const float FPS = 60;
    
    
    int fMain_Loop()
    {
    	ALLEGRO_DISPLAY *display = NULL;
    	ALLEGRO_BITMAP *image = NULL;
    	ALLEGRO_TIMER *timer = NULL;
    	ALLEGRO_EVENT_QUEUE *event_queue = NULL;
    	cList list;
    
    	bool redraw = true;
    	bool doexit = false;
    	
    	//Start of init code
    	if(!al_init()) {
    		al_show_native_message_box(display, "Error", "Error", "Failed to initialize allegro!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
    		return -1;
    	}
    
    	if(!al_init_image_addon()){
    	   al_show_native_message_box(display, "Error", "Error", "Failed to initialize Image Addon!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
          return -1;
       }
    
    	if(!al_install_mouse()) {
    	   al_show_native_message_box(display, "Error", "Error", "Failed to install mouse!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
    	   return -1;
       }
    	//end of init code
    
    	display = al_create_display(800,600);
    	if(!display) {
    	   al_show_native_message_box(display, "Error", "Error", "Failed to create display!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
          return -1;
       }
    
    	event_queue = al_create_event_queue();
    	if(!event_queue) {
    		al_show_native_message_box(display, "Error", "Error", "Failed to create event_queue!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
    		al_destroy_display(display);
    		al_destroy_timer(timer);
    		return -1;
    	}
    
    	image = al_load_bitmap("bitship.png");
       if(!image){
    	   al_show_native_message_box(display, "Error", "Error", "Failed to load image!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
          return -1;
       }
    
       al_draw_bitmap(image,0,0,0);
       al_flip_display();
       al_rest(5.0);
       
    	timer = al_create_timer(1.0 / FPS);
       if(!timer) {
    	   al_show_native_message_box(display, "Error", "Error", "Failed to create timer!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
    	   return -1;
       }
    
       if(!al_install_mouse()) {
    	   al_show_native_message_box(display, "Error", "Error", "Failed to install mouse!", NULL, ALLEGRO_MESSAGEBOX_ERROR);
    	   return -1;
       }
    
       //Regester event stuffs
    
       al_register_event_source(event_queue, al_get_display_event_source(display));
       al_register_event_source(event_queue, al_get_timer_event_source(timer));
       al_register_event_source(event_queue, al_get_mouse_event_source());     
    
       al_start_timer(timer);
    
        while(!doexit)
       {
    	   srand (time(NULL));
    	   int tmp = rand() % 5 + 0;
    	   do{
    		   fprintf(stderr, "trying create star %d\n", tmp);
    		   --tmp;
    		   cStar star;
    		   list.Append(star);
    	   }while(tmp > 0);
    
    	   ALLEGRO_EVENT ev;
    	   al_wait_for_event(event_queue, &ev);
    
    	   if(ev.type == ALLEGRO_EVENT_TIMER) {
    		   redraw = true;
    		   
    	   }
    	   else if(ev.type == ALLEGRO_EVENT_DISPLAY_CLOSE) {
    		   doexit = true;
    	   }
    	   else if(ev.type == ALLEGRO_EVENT_MOUSE_AXES || ev.type == ALLEGRO_EVENT_MOUSE_ENTER_DISPLAY) {
    		 
    	   }
    	   else if(ev.type == ALLEGRO_EVENT_MOUSE_BUTTON_UP) {
    
    	   }
    	   //Personal fun code
    	   if(redraw && al_is_event_queue_empty(event_queue)) {
    		   redraw = false;
    		   al_clear_to_color(al_map_rgb(0,0,0));
    		   list.Update();
    		   list.Render();
    		   
    		   al_flip_display();
    	   }
       }
       
    
    	al_destroy_bitmap(image);
    	al_destroy_display(display);
    	al_destroy_timer(timer);
    	al_destroy_event_queue(event_queue);
    
    	return 0;
    
    }

  6. #6
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    ok I did a step by step and it just jumps over assigning newNode to tmp. its as if the while() line is just a return;

  7. #7
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,825
    I lied.

    Not every list loop uses != NULL as the exit condition.

    Finding where to append a node needs to stop at the last node, not fall off the end.

    Post your latest append() code.


    BTW, why aren't you using std::list to begin with?

    > srand (time(NULL));
    You need to remove all of these, and put a single call to srand() at the start of main.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  8. #8
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    ok YAY :P I will change the random seed to the start of the program. im using namespace std;

    Code:
     void cList::Append(cStar data){
    
    	 cNode* newNode = new cNode();
    	 UID++;
    	 newNode->SetData(data, UID);
    	 newNode->SetNext(NULL);
    
    	 //head->SetData(data,UID);
    
    
    	 cNode *tmp = head;
    	 while ( tmp != NULL ) {
    		tmp = &tmp->Next();
    			}
    	 tmp = newNode;
    
     }
    Thats my latest code

  9. #9
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,825
    > tmp = &tmp->Next();
    What's that & doing there!?

    Code:
    if ( head == NULL ) {
      // special case, list is empty
      head = newNode; 
    } else {
      tmp = head;
      while ( tmp->Next() != NULL ) tmp = tmp->Next();
      // now tmp points to the last node
      tmp->SetNext(newNode);
    }
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  10. #10
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    lol the & was something I was testing just in case I didn't know what i was doing.... and it seems i still dont know what im doing. the program crashes at the end of that function. just to show you the new code here it it


    Code:
     void cList::Append(cStar data){
    
    	 cNode* newNode = new cNode();
    	 UID++;
    	 newNode->SetData(data, UID);
    	 newNode->SetNext(NULL);
    
    	 //head->SetData(data,UID);
    
    
    	 cNode *tmp;
    	 if ( head == NULL ) {
      // special case, list is empty
    	  head = newNode; 
    	} else {
    	  tmp = head;
    	  while ( tmp->Next() != NULL ) tmp = tmp->Next();
    	  // now tmp points to the last node
    	  tmp->SetNext(newNode);
    	}
    
     }
    this is crazy but maybe is has something to do with allegro... I have been working on this problem for days.

  11. #11
    Registered User
    Join Date
    Jul 2013
    Location
    Germany
    Posts
    487
    Code:
    while ( tmp->Next() != NULL ) tmp = tmp->Next();
    Why are you using Next(), shouldn't it be just Next? You are missing some
    Code:
    { }
    in your while statement too.

    If head is NULL shouldn't you set the values then point it to the next node?

  12. #12
    Registered User
    Join Date
    Oct 2010
    Posts
    7
    next is a private var so Next(); is a function that returns the next node.

  13. #13
    Registered User
    Join Date
    Jun 2013
    Posts
    56
    Quote Originally Posted by BlizAce View Post
    next is a private var so Next(); is a function that returns the next node.
    Then it would be much simpler if the class controlling the list did the adding to the list not another class. I think your over complicating the program a bit and thats causing some of your headaches. Think of the car example. The engine handles all of the engine details, the tires don't handle engine details. You need to decide your classes so that they handle any aspect of that class. All other classes should simple call on that class to do what it needs to do.

    For your specific example the append should probably just pass in the data to the list class and let the list class handle putting it into the list. When you want data from the list you pass in what data you want and let the list class tell you if it is there. If you want to remove something from the list you pass in what to remove and let the list class take care of the removal or letting you know it was not found.

    On a side note, stop using:
    Code:
    using namespace std;
    I don't understand why books insist in teaching students this terrible practice.

    This line tells the compiler to copy the entire std namespace into your program, when you more often then not only plan to use a small amount of that namespace.

    instead use something like this:
    Code:
    using std::cout;
    using std::cin;
    using std::endl;
    This tells the compiler whenever it sees cout to use the function from the std namespace and that you will NOT use the variable cout for anything else, etc etc with the other calls.
    In this way you only use and copy in the functions you need to use. This keeps you from bloating up your program needlessly. Small potatoes when you're learning, however when you are making huge projects for companies and size matters, bad habits are bad habits.

  14. #14
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,271
    Quote Originally Posted by Ewiv
    On a side note, stop using:
    Code:
    using namespace std;
    I don't understand why books insist in teaching students this terrible practice.
    No, it isn't terrible, as long as the using directive is within a restricted scope, or at least is only placed in a source file after the last header is included.

    Quote Originally Posted by Ewiv
    This line tells the compiler to copy the entire std namespace into your program, when you more often then not only plan to use a small amount of that namespace.
    No, it doesn't. The using directive makes those names in the std namespace for which declarations are in effect available without qualification (besides via argument dependent lookup) within the scope of the using directive.

    Quote Originally Posted by Ewiv
    This tells the compiler whenever it sees cout to use the function from the std namespace and that you will NOT use the variable cout for anything else, etc etc with the other calls.
    You might need to make up your mind as to whether std::cout is a function or a variable
    More importantly, the latter is not quite true: it is still possible to say, declare another variable named cout within a more local scope. Usually a bad idea, but possible.

    Quote Originally Posted by Ewiv
    This keeps you from bloating up your program needlessly.
    It isn't bloat. Well, okay, in a way it is in that the space of all unqualified names gets "bloated" more than when you sparingly use using declarations instead, but when the scope of the using directive is small such that only a few names need to be considered, then that is of no concern. Furthermore, where there might be ambiguity, it is still possible to fully qualify the specific potentially ambigious name.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  15. #15
    Registered User
    Join Date
    Jun 2013
    Posts
    56
    I guess I was not the most clear in that one line

    std::cout is a function I know this I was trying to say that, what I was implying is you will not do something like

    int cout; at some point in your program.

    Thanks for the correction though, I am always looking to learn more.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 13
    Last Post: 09-22-2013, 11:34 PM
  2. Linked List Issues
    By Sorinx in forum C Programming
    Replies: 13
    Last Post: 12-10-2012, 02:19 PM
  3. single linked list to double linked list (help)
    By Countfog in forum C Programming
    Replies: 8
    Last Post: 04-29-2008, 09:04 PM
  4. singly linked list to doubly linked list
    By t48j in forum C Programming
    Replies: 3
    Last Post: 03-23-2005, 06:37 PM
  5. Replies: 6
    Last Post: 03-02-2005, 02:45 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21