Help! i been looking for the dangling pointer can not find it

[nameishi]

#include<iostream>
#include<fstream>
#include<cmath>
#define ofile "log.txt"
#define ifile "integer.dat"

using namespace std;

struct countNode {
	int num;
	int even=0;
	countNode*prev;
	countNode*next;
};
countNode*head, *prev, *tail, *current;
void storeData( ifstream&, ofstream&);
void evenData(ifstream&, ofstream&);
void evenData(ifstream&infile, ofstream&outfile) {
	int value = 0;
	current = head;
	while (current != tail) {
		value = current->num;
		if (value % 2 == 0) {
			current->even = value;
			current = current->next;
		}
		else
		{
			current->even = NULL;
			current = current->next;
		}

	}
}
void storeData(ifstream&infile, ofstream&outfile) {
	head = new countNode;
	int value = 0;

	countNode*current, *previous, *tail, *temp;
	 temp = new countNode;
	if (head == NULL) {//to check if file is empty after the you call the store function method
		cout << "File is empty" << endl;
		outfile << "File is empty" << endl;
	}
	infile >> value;
	while(value < 1) {//CHECKS that the head stores int > 1
		cout << "Error: integer is less than 1 " << endl;
		outfile << "Error: integer is less than 1 " << endl;
		infile >> value;
	}
	head->num=value;//stores first number
	current = head;
	previous = head;
	head->prev = NULL;
	while (!infile.eof()) {
		temp = new countNode;
		infile >> value;
		while (value < 1) {
			cout << "Error: integer is less than 1 NODE " << endl;
			outfile << "Error: integer is less than 1 NODE" << endl;
			infile >> value;
		}
		temp->num=value;//stores next valid value
		if (previous->num > temp->num) {//in case the next value is less than the Head value
			temp->next = previous;
			previous->prev = temp;
			head = temp;
			head->prev = NULL;
		}
		while (current != NULL && previous->num <= temp->num) {//comparing new value to the beginning of the list to end
			current = head;//brings current to the front and previous
			previous = current;
			temp->next = head;
			while (temp->num > current->num) {//error: dangling pointer here NOT SURE WHY?
				previous = current;
				current = current->next;
				if (current = NULL) {//if temp# is greater than all other Node# then creates new node and stores it
					current->next = new countNode;
					current = current->next;
					current->num = temp->num;
					current->prev = previous;
				}
			}
				if (temp->num < current->num)//if temp finds a current value that is Greater than itself
				{//once temp gets lower than current will it break the loop??
					temp->next = current;//putting temp in between previous and Current in the List
					current->prev = temp;
					temp->prev = previous;
					previous->next = temp;
				}
				if (temp->num == current->num) {
					delete temp;
				}
			
		}
		
		}
	
	tail = current;
	tail->next = NULL;
	current = head;
	cout << "all integers in the file : " << endl;
	while (current!= NULL) {//use this for printing
		
		cout<<current->num<<endl;
		current = current->next;//moves current to next node
	}
	evenData(infile, outfile);//goes to sort nodes to even
}

int main() {
	ifstream infile;
	ofstream outfile;
	
	char letter;
	
	infile.open("integer.dat");
	outfile.open("log.txt");

	if (infile.fail()) {
		cout << "Error opening file OUT " << endl;
		return 1;
	}
	if (outfile.fail()) {
		cout << "Error opening file IN " << endl;
		return 1;
	}
	cout << "Welcome to my integer sorting program!" << endl;
	storeData(infile, outfile);//error no idea
	
	
	
		
	infile.close();
	outfile.close();
	system("pause");
	return 0;
}

[TheIdeasMan]

Hi,

I see 4 new and 1 delete :+) Each new must have a corresponding delete.

Some other things:

You have too much code in your functions, there is kind of a rule that says that functions should be no more than 40LOC, some go for even less like 20. Candidates for functions are compound statements found in loops, if etc. Doing this will aid understanding, and make the code easier to read.

Don't loop on eof, use the stream instead:

while (infile) {

The steam will return a 0 if it has problems, your method might attempt to go past the end of the file.

I had these warnings, when compiling with the gear icon, top right of your code, with all the warnings on:

In function 'void evenData(std::ifstream&, std::ofstream&)': 
29:18: warning: converting to non-pointer type 'int' from NULL [-Wconversion-null] 
At global scope: 
18:24: warning: unused parameter 'infile' [-Wunused-parameter] 
18:41: warning: unused parameter 'outfile' [-Wunused-parameter] 
In function 'void storeData(std::ifstream&, std::ofstream&)': 
77:23: warning: suggest parentheses around assignment used as truth value [-Wparentheses] 
In function 'int main()': 
115:7: warning: unused variable 'letter' [-Wunused-variable]

Always set your warnings to their highest level, and don't ignore them.

Line 29: use nullptr , seen as even is a pointer
Line 77: Use == for equality comparison, not = for assignment

Finally, what I seem to say to everyone: Don't have using namespace std; - it defeats the purpose of name spaces and will cause trouble one day. Put std:: before each std thing, believe me it's the easiest way in the end, all the experts do this. Google this subject.

Hope all goes well :+)

[nameishi]

TheIdeasMan, thanks for the response but why do i need a delete and new node 1:1 ratio? i am trying to add nodes to the list? and im not sure but did you happen to get an error of a dangling pointer on line 76
while(temp->num >current->num)

[TheIdeasMan]

Hi

but why do i need a delete and new node 1:1 ratio?

Otherwise you leak memory. Call delete when you are finished with the resource, this might as late as at the end of main() but could be elsewhere - for example if you remove a node.

and im not sure but did you happen to get an error of a dangling pointer on line 76

I compiled using the gear icon at the top right of your code with all the warning levels turned on. Only had the warnings I posted. But you obviously have other errors, can you post them here exactly as shown?

Have you fixed up the other things I mentioned?

Have you tried a debugger? It is often the easiest way, provided you have an executable program.

I notice you have NULL everywhere, that might cause problems: NULL is 0, but you are comparing it to pointers. Use nullptr

Also, you appear to allocate memory with new, but don't initialise anything associated with that pointer. Line 36 to 41 for example.

And you seem to have uninitialised variables everywhere.

You have these global pointers:

countNode*head, *prev, *tail, *current;

But these local ones inside storeData

countNode*current, *previous, *tail, *temp;

I find that confusing, another good reason not to have global data. If you want to use the global ones, pass them to the function as argulments.

When you get to EvenData the global ones are being used, not related to the other ones.

Anyway, I hope this helps - but I think you hav quite a bit to do to fix things up :+)

[TheIdeasMan]

Hello again,

With the idea of functions, I think you really need functions to do the following things:

* Create an empty List
* Insert into list - either beginning, middle or end (push_front , insert, push_back)
* Print the whole list
* Remove something from the list
* Find something in the list

The other thing about functions is that they should only do one conceptual thing. This helps with keeping things easy to understand. In turn this will help you to write better code with less chance of errors, and it will be easier to find an error if you do have one.

So sorry to pour freezing cold water all over your code, but I really would suggest starting again. I know that sounds bad, you probably spent quite some time doing your code, but consider the benefits of having done it properly :+) , as opposed to fixing up something that is bad.

Regards

[nameishi]

-Thanks i forgot to pass my Nodes through the functions so i fixed that and deleted excess Nodes.
-When you see NULL is because I am using a doubly linked list so my previous* has to point to NULL if it is in the beginning of the list, in the other case is for when i am attempting to add a Node in front of the head node, therefore that new head->prev must point to NULL
-I get an error when i use nullptr vs NULL so thats why i stuck with node..i figured it is a compiler preference.
-The error that i believe is a dangling pointer says:
<Exception thrown at 0x00BBAE7E in lab6 CIS 200.exe: 0xC0000005: Access violation reading location 0xCDCDCDCD.

If there is a handler for this exception, the program may be safely continued.
click BREAK CONTINUE RETRY>

yellow arrow points to line 76 and in the separate window below it gives me the values of my variables. I have no idea how this helps me
current 0xcdcdcdcd {num=??? even=??? prev=??? ...} countNode *

(red)head 0x00d29ed0 {num=1 even=0 prev=0x00000000 <NULL> ...} countNode *

(red)previous 0x00d29008 {num=-842150451 even=0 prev=0x00d29ed0 {num=1 even=0 prev=0x00000000 <NULL> ...} ...} countNode *

(red)temp 0x00d2d940 {num=8 even=0 prev=0xcdcdcdcd {num=??? even=??? prev=??? ...} ...} countNode *

[cire]

-I get an error when i use nullptr vs NULL so thats why i stuck with node..i figured it is a compiler preference.

Are you using an old version of VC++? If so, consider upgrading.


http://stackoverflow.com/questions/8275418/with-c-i-get-pointer-with-0xcdcdcdcd-when-creating-a-class-what-is-happenin

Your pointer is uninitialized and has never held a valid value. Knowing that and tracing through storeData where your list is built, one can see that the first node allocated doesn't have the next pointer set to anything anywhere.

[nameishi]

I re wrote the method...is this better?

void storeData(ifstream&infile, ofstream&outfile, countNode*head, countNode *previous, countNode*tail, countNode*current) {
	countNode*temp;
	int value = 0;
	head = NULL;
	temp = head;
	while (!infile.eof()) {
		infile >> value;
		while (value < 1) {
			outfile << "Error number is less than 1 \n";
			infile >> value;
		}
		if (head == NULL) {
			head = new countNode;
			head->num = value;
			head->prev = NULL;
			head->next = NULL;
			tail = head;
		}
		else
			if (value < head->num) {
				temp = new countNode;
				temp->num = value;
				temp->next = head;
				head = temp;
				head->prev = NULL;
			}
			else {
				previous = head;
				current = head->next;
				
			}
			while ((current->next != NULL) && (current->num <= value)) {//this does not store the greater value
				previous = current;
				current = current->next;//now in the same as temp

			}
			if (current->num == value) {
				break;//if equal exit while loop
			}
			else
			{
				temp = new countNode;
				temp->num = value;
				previous->next = temp;
				temp->next = current;
			}
	}
}

[cire]

I re wrote the method...is this better?

If it doesn't crash for you, I suppose it's better.

head, tail, previous and current should not be arguments to the function.

As in the original function, you're leaking memory and looping incorrectly on eof.
while (infile >> value) { would be a better loop condition (assuming you nix line 7.)

Lines 8 through 10 don't really have any reason to be there if your expectation is that you're reading from a file and not doing interactive input. if (value < 1) continue ;

[TheIdeasMan]

Hi,

Just to reiterate:

http://www.cplusplus.com/forum/general/178438/#msg878741

Consider it, seriously :+)

[nameishi]

can you push and pop linked lists? thought just vectors and arrays

i have to use a doubly linked list

[TheIdeasMan]

Hi,

can you push and pop linked lists? thought just vectors and arrays

i have to use a doubly linked list

If you look at std::list , here:

http://www.cplusplus.com/reference/list/list/

You can see on the left all the member functions. It is a double linked list.

[nameishi]

Thanks allot i figured out the issue with the code, i want referencing current = head for my first condition. Drawing link lists out help allot.. Thanks for all the input.