C++ array getting corrupt

[Guest]

I'm reading some files & putting the data into an array.

For some reason after the static method finishes executing, visual studio throws an exception saying that the array is corrupt. I'm not quite sure why it's doing this.

 

Notes:
vector is initialized & filled with 100 initialized values before ReadFile is called. tileData is the array giving me problems.

My file format has 3 numbers that represent 1 "Tile" for this program. Semi colons seperate values. New lines are just for neat formatting.

 

hlcFiles.cpp (In edit I have the most recent revision)

Spoiler

#include <iostream>
#include <fstream>

#include "hlcFiles.h"
#include "Tile.h"


void Files::ReadFile(fstream &fs, vector<Tile> &tiles) {
	char ch;
	int tileDataIndex = -1;
	int vectorIndex = 0;

	int tileData[3] = {-1, -1, -1}; //Array having problems. 

	while (!fs.eof()) {
		fs.get(ch);
		cout << ch;
		tileData[tileDataIndex] = Interpret(ch, tileDataIndex, vectorIndex);
		
		//if (tileDataIndex > 3) {
			cout << endl << "Tile " << vectorIndex << " tile index " << tileDataIndex << endl;
			//tiles[vectorIndex].ChangeType(tileData);
		//}
	}
	cout << endl << "File read." << endl; //This line executes. 
}//Line where error occurs. 

int Files::Interpret(char &ch, int &tileDataIndex, int &vectorIndex) {
	if (ch == '\n') {
		return -1;
	} else if (ch == ';') {
		tileDataIndex = -1;
		vectorIndex += 1;
		return -1;
	} else {
		try {
			if ((int)ch > 48 || (int)ch <= 57) {//Assures ASCII value is a number. 0-9
				tileDataIndex += 1;
				int returnVariable = (int)ch - 48;
				return returnVariable;
			}
			else {
				int foo;
				cout << endl << "ERROR! Value of character is not a digit." << endl;
				cin >> foo;
				return -1;
			}
		}
		catch (const exception& e) {
			int foo;
			cout << e.what() << endl;
			cin >> foo;
			return -1;
		}
	}
}

void Files::WriteFile(fstream &fs, vector<Tile> tiles) {

}

 

hlcFiles.h

Spoiler

#pragma once
#include <iostream>
#include <fstream>

#include "Tile.h"

using namespace std;

class Files {
private:
	static int Interpret(char &ch, int &tileDataIndex, int &vectorIndex);
	//static int index;
	//static int tileData[3];

public: 
	static void ReadFile(fstream &fs, vector<Tile> &tiles); //fs stands for file stream
	static void WriteFile(fstream &fs, vector<Tile> tiles);


};

 

Text file being read (.txt):

Spoiler

109;513;
300;456;

I have no idea why the array is getting corrupted.

For parsing the ch to an integer in the try, I've tried

(int)ch

(int)ch - 0

(int)ch - 48

ch - 0

ch - 48

 

EDIT:
hlcFiles.cpp

Spoiler

#include <iostream>
#include <fstream>

#include "hlcFiles.h"
//#include "Tile.h"


void Files::ReadFile(fstream &fs, vector<Tile> &tiles) {
	char ch;
	int tileDataIndex = 0;
	int vectorIndex = 0;

	int tileData[3] = {-1, -1, -1};

	while (!fs.eof()) {
		fs.get(ch);
		cout << ch;
		if (tileDataIndex < 3) {
			tileData[tileDataIndex] = Interpret(&ch, &tileDataIndex, &vectorIndex);
			if (ch != ';' && ch != '\n')
				tileDataIndex += 1;
		}
		else {
			cout << endl;
			Interpret(&ch, &tileDataIndex, &vectorIndex);
		}
		if (tileDataIndex >= 3) {
			cout << endl << "Tile " << vectorIndex << " tile index " << tileDataIndex << endl;
			tiles[vectorIndex].ChangeType(tileData);
		}
	}

	cout << endl << "File read." << endl;
}

int Files::Interpret(char * ch, int  * tileDataIndex, int  * vectorIndex) {
	if (*ch == '\n') {
		return -1;
	} else if (*ch == ';') {
		cout << endl;
		*tileDataIndex = 0;
		vectorIndex += 1;
		return -1;
	} else {
		try {
			if ((int)ch > 48 || (int)ch <= 57) {
				
				int returnVariable = (int)ch;// -48;
				return returnVariable;
			}
			else {
				int foo;
				cout << endl << "ERROR! Value of character is not a digit." << endl;
				cin >> foo;
				return -1;
			}
		}
		catch (const exception& e) {
			int foo;
			cout << e.what() << endl;
			cin >> foo;
			return -1;
		}
	}
}

void Files::WriteFile(fstream &fs, vector<Tile> tiles) {

}

 

hlcFiles.h

Spoiler

#pragma once
#include <iostream>
#include <fstream>

#include "Tile.h"

using namespace std;

class Files {
private:
	static int Interpret(char * ch, int  * tileDataIndex, int  * vectorIndex);
	//static int index;
	//static int tileData[3];

public: 
	static void ReadFile(fstream &fs, vector<Tile> &tiles); //fs stands for file stream
	static void WriteFile(fstream &fs, vector<Tile> tiles);


};

 

 

z

[Mira Yurizaki]

You have this:

int tileDataIndex = -1;

But then the next usage of that value I see is this:

tileData[tileDataIndex] = Interpret(ch, tileDataIndex, vectorIndex);

I'm not sure if this is causing it (accessing an array using a negative index), but this looks strange to me.

 

EDIT: Oh, you're passing it in to the function that's supposed to do something with it. But it was expecting an address, and instead you passed a value.

 

EDIT 2: Are

char &ch, int &tileDataIndex, int &vectorIndex

 

Supposed to be pointers? Because if they are, I think you should be using pointer syntax. This is throwing me off hard.

[mariushm]

make sure your enters are all just newline/LF  ( \n or 0x0A ) , and not CRLF 0x0D , 0x0A

 

You could also try to read character by character, keep reading until character is not 0..9, and if the character is within 0..9, multiply the value being read by 10 and add the character ... when first non 0..9 is read, add the value to array and reset value to 0.

 

something like this

 

value = 0;

while not eof (filehandle)

  $charcode = read char from file handle;

  if ( charcode >='0' and charcode <= '9')  {

    value = value * 10 +   charcode ascii code - 48;

 } else {

    if value != 0  { // found non numeric character, and there were numeric characters before, so a number ended before

       // add value to array

      value = 0; // reset

    }

   // otherwise, it's probably newline, ; crap characters

}

[BobVonBob]

2 minutes ago, Mira Yurizaki said:

You have this:

int tileDataIndex = -1;

But then the next usage of that value I see is this:

tileData[tileDataIndex] = Interpret(ch, tileDataIndex, vectorIndex);

I'm not sure if this is causing it (accessing an array using a negative index), but this looks strange to me.

Adding on to this, I believe the behavior this would cause is an implicit conversion to a size_t which is a 64 bit unsigned int (on 64 bit systems), and accessing the array location there, at tileData[2^64-1].

[Guest]

3 minutes ago, Mira Yurizaki said:

EDIT: Oh, you're passing it in to the function that's supposed to do something with it. But it was expecting an address, and instead you passed a value.

Is there a recommended way to access the value?

 

I tried putting variables in the Header but I couldn't access the variables in any of the functions since the class is never initialized to an object.

 

I changed the CPP to be:
 

int tileDataIndex = 0;

with the incriment moved just under the = like this:
 

tileData[tileDataIndex] = Interpret(ch, tileDataIndex, vectorIndex);
tileDataIndex += 1;

 

4 minutes ago, mariushm said:

make sure your enters are all just newline/LF  ( \n or 0x0A ) , and not CRLF 0x0D , 0x0A

I'm not sure what you mean. I have an if that determines if there is a new line:
 

if (ch == '\n')

4 minutes ago, mariushm said:

You could also try to read character by character,

I do that.

fs.get(ch); //fs is type fstream

4 minutes ago, mariushm said:

multiply the value being read by 10 and add the character ... when first non 0..9 is read, add the value to array and reset value to 0.

 

Could you clarify in some C++. I'm not sure I follow.

[Guest]

1 minute ago, BobVonBob said:

Adding on to this, I believe the behavior this would cause is an implicit conversion to a size_t which is a 64 bit unsigned int (on 64 bit systems), and accessing the array location there, at tileData[2^64-1].

I have attempted to edit that in my quote to M Yuri.

The complete update to the file is in the edit section of my OP now.
Still getting error:
 

Quote

Run-Time Check Failure #2 - Stack around the variable 'tileData' was corrupted.

 

[Mira Yurizaki]

5 minutes ago, fpo said:

Is there a recommended way to access the value?

If this is a class method, then make the index a member. Otherwise you use a pointer. The "proper" way to do this is:

void some_main(){
  int foobar = -1

  ...

  somefunc(&foobar);

  ...
}
  
void somefunc(int * fizzbuzz){
  ...
}

I'll have a better look at what's going on when I can, but I think there are issues with the design of the original code.

[BobVonBob]

3 minutes ago, fpo said:

-snip-

You're still setting tileDataIndex to -1 inside Interpret if the character you read is ';'

[Guest]

2 minutes ago, Mira Yurizaki said:

If this is a class method, then make the index a member. Otherwise you use a pointer. The "proper" way to do this is:

 

I tried implemented that & changed variable calls to have a star in front of the, but I still get the error:
 

Quote

Run-Time Check Failure #2 - Stack around the variable 'tileData' was corrupted.

[Mira Yurizaki]

13 minutes ago, fpo said:

I tried implemented that & changed variable calls to have a star in front of the, but I still get the error:

I looked into what's going on some more and the biggest issue with the design is Interpret is manipulating tileDataIndex. This is beyond the scope of what I feel the method needs to be doing. Especially because in the "failure" cases, the method is putting it back to -1, which causes an negative index access to an array.

 

Redesign the methods so Interpret is not manipulating tileDataIndex first.

 

EDIT: The best way to put it is either Interpret populates the array in its entirety, or it returns a single value based on some input, but not involve mainpulating anything related to the array (include an index variable) in any shape or form.

Edited February 4, 2019 by Mira Yurizaki

[BobVonBob]

I believe you also over-run the end of tileData by either 1 or 2 depending on where you are in the txt file, since you also insert -1 into tileData for ';' and '\n'

 

EDIT: I think this can be fixed by not doing anything to tileDataIndex or vectorIndex inside Interpret, but instead getting the return value of Interpret, and only assigning a value to the array if the value returned by Interpret isn't -1.

[Guest]

5 minutes ago, Mira Yurizaki said:

I looked into what's going on some more and the biggest issue with the design is Interpret is manipulating tileDataIndex. This is beyond the scope of what I feel the method needs to be doing.

I created a new Method (Incriment) to handle that information now using pointers.

Spoiler

void Files::Incriment(char * ch, int * tileDataIndex, int * vectorIndex) {
	if (*ch != ';' && *ch != '\n')
		*tileDataIndex += 1;

	else if (*ch == ';' || *ch == '\n') {
		*tileDataIndex = 0;
		*vectorIndex += 1;
	}

}

 

5 minutes ago, Mira Yurizaki said:

 

EDIT: The best way to put it is either Interpret populates the array in its entirety, or it returns a single value based on some input, but not involve mainpulating the array in any shape or form.

 

I have it setup so that ReadFile figures out what index we're working with & then Interpret finds the value & returns it to ReadFile.
ReadFile takes the returned value and applies it to the intended index of the array.

 

 

19 minutes ago, BobVonBob said:

I believe you also over-run the end of tileData by either 1 or 2 depending on where you are in the txt file, since you also insert -1 into tileData for ';' and '\n' 

I have changed this so we no longer work with negatives in the index.

I have -1 only for values. This is a second error check to make sure the correct values are placed inside. If negative values appear later in the program, I'll have error checks to say that to help determine the issue.

 

 

 

I think the problem is solved with several logical changed.

hlcFiles.cpp

Spoiler

#include <iostream>
#include <fstream>

#include "hlcFiles.h"
//#include "Tile.h"


void Files::ReadFile(fstream &fs, vector<Tile> &tiles) {
	char ch;
	int tileDataIndex = 0;
	int vectorIndex = 0;

	int tileData[3] = {-1, -1, -1};

	while (!fs.eof()) {
		fs.get(ch);
		cout << ch;

		if (tileDataIndex < 3) {
			tileData[tileDataIndex] = Interpret(&ch, &tileDataIndex, &vectorIndex);
		}

		Incriment(&ch, &tileDataIndex, &vectorIndex);

		if (tileDataIndex >= 3) {
			cout << endl << "Tile " << vectorIndex << " tile index " << tileDataIndex << endl;
			tiles[vectorIndex].ChangeType(tileData);
		}

		if (tileDataIndex >= 3) {
			cout << "Ran0" << endl;
			Interpret(&ch, &tileDataIndex, &vectorIndex);
		}
	}

	cout << endl << "File read." << endl;
}

void Files::Incriment(char * ch, int * tileDataIndex, int * vectorIndex) {
	if (*ch != ';' && *ch != '\n')
		*tileDataIndex += 1;

	else if (*ch == ';' || *ch == '\n') {
		*tileDataIndex = 0;
		*vectorIndex += 1;
	}

}

int Files::Interpret(char * ch, int  * tileDataIndex, int  * vectorIndex) {
	if (*ch == '\n') {
		return -1;
	} else if (*ch == ';') {
		//*tileDataIndex = 0;
		//*vectorIndex += 1;
		return -1;
	} else {
		try {
			if ((int)ch > 48 || (int)ch <= 57) {
				
				int returnVariable = (int)ch;// -48;
				return returnVariable;
			}
			else {
				int foo;
				cout << endl << "ERROR! Value of character is not a digit." << endl;
				cin >> foo;
				return -1;
			}
		}
		catch (const exception& e) {
			int foo;
			cout << e.what() << endl;
			cin >> foo;
			return -1;
		}
	}
}

void Files::WriteFile(fstream &fs, vector<Tile> tiles) {
	//if oneLine (sqrt(tiles.size())%10 == 0), new line. 
}

 

hlcFiles.h

Spoiler

#pragma once
#include <iostream>
#include <fstream>

#include "Tile.h"

using namespace std;

class Files {
private:
	static int Interpret(char * ch, int  * tileDataIndex, int  * vectorIndex);
	static void Incriment(char * ch, int * tileDataIndex, int * vectorIndex);
	//static int index;
	//static int tileData[3];

public: 
	static void ReadFile(fstream &fs, vector<Tile> &tiles); //fs stands for file stream
	static void WriteFile(fstream &fs, vector<Tile> tiles);


};

 

 

However now I'm getting exceptions in other parts of the program where I don't think I should.

 

tiles[vectorIndex].ChangeType(tileData);

Calls a method in Tile's class called ChangeType. This changes the values of an array that is not used anywhere else in the program.
I'm getting an error in DrawTile however. The final line at the close brace where I wasn't getting errors before. There's nothing wrong with my drawing logic as the SFML all works properly & draws things how I want them to. Just when I call the above method, I get an issue.
perhaps I'm passing the array incorrectly?

 

This link seems to show I'm doing it properly however:

https://www.tutorialspoint.com/cplusplus/cpp_passing_arrays_to_functions.htm

Spoiler

Quote

 

double getAverage(int arr[], int size) {
  int i, sum = 0;       
  double avg;          

   for (i = 0; i < size; ++i) {
      sum += arr[i];
   }
   avg = double(sum) / size;

   return avg;
}

#include <iostream>
using namespace std;
 
// function declaration:
double getAverage(int arr[], int size);

int main () {
   // an int array with 5 elements.
   int balance[5] = {1000, 2, 3, 17, 50};
   double avg;

   // pass pointer to the array as an argument.
   avg = getAverage( balance, 5 ) ;
 
   // output the returned value 
   cout << "Average value is: " << avg << endl; 
    
   return 0;
}

 

 

Tile.cpp

Spoiler

#include <iostream>
#include "Tile.h"
#include "Globals.h"
#include "SFML/Graphics.hpp"

Tile::Tile(float xInput, float yInput) {
	x = xInput;
	y = yInput;
	rect.setSize(RECTsIZE);
	
	
	for (int i = 0; i < sizeof(type); i++) {
		type[i] = 0;
	}

	Color Brown (70.0f, 15.0f, 0.0f);
	rect.setFillColor(Brown);
	//rect.setFillColor(Color(255, 0, 0));
	
	//GetCoords();
	
}

void Tile::DrawTile(RenderWindow &wind, float wholeBox) {
	float halfBox = wholeBox / 2.0f;
	rect.setOrigin(halfBox, halfBox);
	rect.setPosition(x, y);
	wind.draw(rect);
}
void Tile::GetCoords() {
	std::cout << "x " << x << " y " << y << std::endl;
}
void Tile::ChangeColour(Color newColour) {

}
void Tile::ChangeType(int input[3]) {
	for (int i = 0; i < sizeof(type); i++) {
		type[i] = input[i];
	}
}

 

Tile.h

Spoiler

#pragma once
#include <SFML/Graphics.hpp>
using namespace sf;
class Tile {
public:
	Tile(float xInput, float yInput);
	void DrawTile(RenderWindow &wind, float wholeBox);
	void ChangeColour(Color newColour);
	void GetCoords();
	void ChangeType(int input[3]);
protected:

private:
	float x, y;
	int type[3];
	Color colour;

	RectangleShape rect;




};

 

 

 

Thank you all for your help! I greatly appreciate it.

[Mira Yurizaki]

I think it's easier for me at this point to refactor this code to what I think it's trying to do:

#include <iostream>
#include <fstream>

#include "hlcFiles.h"
//#include "Tile.h"


void Files::ReadFile(fstream &fs, vector<Tile> &tiles) {
	char ch;
	int tileDataIndex = 0;
	int vectorIndex = 0;

	int tileData[3] = {-1, -1, -1};

	while (!fs.eof()) {
		fs.get(ch);
		cout << ch;
        
        switch(ch){
            case ';':
            case '\n':
                tileDataIndex = 0;
                vectorIndex ++;
                break;
            default:
                if ('0' <= ch && ch <= '9') {
                    tileData[tileDataIndex] = ch;
                }
                else {
                    cout << endl << "ERROR! Value of character is not a digit." << endl;
                    tileData[tileDataIndex] = -1;
                }
                tileDataIndex++
                break;
        }


		if (tileDataIndex >= 3) {
			cout << endl << "Tile " << vectorIndex << " tile index " << tileDataIndex << endl;
			tiles[vectorIndex].ChangeType(tileData);
            
            cout << "Ran0" << endl;
			//Interpret(&ch, &tileDataIndex, &vectorIndex); //?
		}
	}

	cout << endl << "File read." << endl;
}

void Files::WriteFile(fstream &fs, vector<Tile> tiles) {
	//if oneLine (sqrt(tiles.size())%10 == 0), new line. 
}

(the code tag thing unfortunately has issues with preserving indents)

 

What's different than the original code:

• Figure out the action to perform based on what ch is first, rather than do all of the actions anyway then figure out within those actions if it needs to be performed based on what ch is. This is what the switch-case statement is doing.
• ch is testing the chars that are valid, rather than the ASCII codes. This is easier to read/understand.
• I removed the type casting when assigning tileData a char. I know in C it isn't necessary to cast a smaller sized data type to a larger sized one, but I'm not sure if C++ does this too (it may based on https://en.cppreference.com/w/cpp/language/implicit_conversion)
• Removed "Interpret", didn't think it needed its own method
• Removed the exception handling because I can't see what would cause an exception in the first place. The only thing I can think of is the type casting of char to int, but this is guaranteed to work.
• Removed Incriment because the switch-case statement handles what it was doing.
• Combined the two if (tileDataIndex >= 3) blocks
• Commented out the last Interpret call mostly as a question, since I don't really know what that's trying to accomplish.

[Guest]

8 minutes ago, Mira Yurizaki said:

I think it's easier for me at this point to refactor this code to what I think it's trying to do:

That's exactly what I was trying to do but neater.

8 minutes ago, Mira Yurizaki said:

• ch is testing the chars that are valid, rather than the ASCII codes. This is easier to read/understand.

For some reason when I tried that it didn't work. I think it's because I used double quotes.

8 minutes ago, Mira Yurizaki said:

• Removed the exception handling because I can't see what would cause an exception in the first place. The only thing I can think of is the type casting of char to int, but this is guaranteed to work.

I was trying to force a way to prevent letters from being entered. Your method solves that anyway.

 

 

I'm having that odd access violation though. stack overflow says I'm attempting to access restricted memory. Or my hardware is faulty.

Microsoft says it's memory corruption.

Quote

Exception thrown at 0x00007FFB5DDCACA6 (sfml-graphics-d-2.dll) in CPPMapTool.exe: 0xC0000005: Access violation reading location 0xFFFFFFFFFFFFFFFF. occurred

 

[Mira Yurizaki]

12 minutes ago, fpo said:

I was trying to force a way to prevent letters from being entered. Your method solves that anyway.

chars are automatically promoted to numbers because the char maps to a numerical code. char '0' is 48 and vice versa. So even if the character was a letter, it'd still be valid to insert into the array.

 

e.g., you can do:

int foobar = 'A';
cout << foobar << endl; // Prints 65

 

Quote

I'm having that odd access violation though. stack overflow says I'm attempting to access restricted memory. Or my hardware is faulty.

Microsoft says it's memory corruption.

It's trying to read from address -1 somewhere still.

[Unimportant]

3 hours ago, Mira Yurizaki said:

If this is a class method, then make the index a member. Otherwise you use a pointer. The "proper" way to do this is:

void some_main(){
  int foobar = -1

  ...

  somefunc(&foobar);

  ...
}
  
void somefunc(int * fizzbuzz){
  ...
}

I'll have a better look at what's going on when I can, but I think there are issues with the design of the original code.

C++ supports references, which allow passing objects by reference without pointer syntax:

void
Foo(int& i)
{
	i = 1337;
}

//...
int a = 101;
Foo(a);
//a now equals 1337

 

[Unimportant]

@fpo

You're overusing references and using them in ways that are frowned upon. The most prominent use of references is to pass a expensive-to-copy object as read only. For example:

//PlayAudioSample signature...
void PlayAudioSample(const AudioSample& Sample);

AudioSample TestSample("test.wav");
PlayAudioSample(TestSample);

Here, AudioSample is a class that reads a wave file from disk and holds all the relevant data. The function PlayAudioSample plays back such samples. We don't want to needlessly copy a potentially expensive object just to play it back, so we pass by reference. PlayAudioSample also does not need to be able to modify the AudioSample (in fact, we want to protect ourselves from accidentally modifying it) so we pass as const reference.

 

The second use of references should be if we want to modify the parameters that are passed, but only in intuitive ways.

The standard library has a great example that goes something like this:

void swap(T& a, T& b);

That swaps the contents of a and b, that's pretty clear and concise and off course requires non-const references.

 

However, in your code:

tileData[tileDataIndex] = Interpret(ch, tileDataIndex, vectorIndex);

I totally don't expect the Interpret function to go and start messing with tileDataIndex and vectorIndex, that's just a huge code smell. More particularly you should not be micromanaging a function's local variables in a subfunction.

It's a huge indication of poor program structure and it's the primary reason you're getting into trouble here. I'm having a hard time following your code and I've been doing this for decades.

 

What is it you're trying to do *exactly* and perhaps we could show you a proper example to pick apart.

 

On a sidenote: Read up about "const correctness" and start applying it rigorously, it'll come back to bite you in the ass if you don't. Trust me, retrofitting existing code for const correctness is a nightmare, and your code won't play well with someone else's code that has const correctness otherwise.

[Guest]

1 hour ago, Unimportant said:

@fpo

You're overusing references and using them in ways that are frowned upon. The most prominent use of references is to pass a expensive-to-copy object as read only. For example:

I'll update my code when I have more time to clean that up.
I actually read your entire post before so I'll just summarize it with that. I'll use less references in the future. When I took my C++ class I used a ton of references because I thought it was objectively better than passing by value as I had the power I needed & as long as I knew I wasn't going to modify it, I'd never have any issues.
I'll clean up my code & look into what you've mentioned.

I spent some time working with my code & I changed out the array in the Tile files to be 3 separate integer variables. That effectively solved my issues. I asked for some help in the SFML Discord as it seemed to be an SFML problem but it was something with arrays. My original C# program did it that way too.

[Guest]

Thanks to everyone that helped me or reacted to good posts!
I feel bad having to pick a single best answer as you all always provide great answers with abundances of informative text for me!

Thanks to you all!