Why does my function keep looping in c++?

[Sauron]

2 minutes ago, Epimetheus said:

jesus christ man why are you being this rude.

I'm not being rude. I'm trying to get you to see that asking questions this way won't get you a useful answer.

3 minutes ago, Epimetheus said:

Is copying and pasting code a difficult thing to do?

If it's just a matter of copying and pasting code then why did you feel the need to ask? There are people here trying to help you and you have no consideration for their efforts.

4 minutes ago, Epimetheus said:

And yeah, it was pretty stupid on my part to write 1 instead of 0, but since i'm a noob with threading, i though the error was in the code itself. And my debugging efforts were mainly just proving it looped, something I stated in the original post

How did you prove it was looping? Again, if you had just posted your output this would have been solved in seconds. Instead we had to go through about a dozen posts of you trying to clarify and getting defensive when given advice on making your code better overall.

 

 

^^^ Not everything applies to you directly, but the output part definitely does and it's good advice in general.

[Eigenvektor]

I have to side with @Sauron on this one. I would've probably worded it a little differently, I didn't personally mind puzzling it out, but I think his answer is absolutely correct. Being able to figure out why your code doesn't work is a very important skill to learn. And learning how to do so effectively will be important when you write code for a living. You don't always have the luxury (or time) to ask co-workers to help you.

 

If your program doesn't work as expected, adding more output to see what it does (or using a debugger) is a great way to understand what it does and why it does it. It's usually a lot more effective than just looking at the code. Especially when your codebase grows to contain hundreds of thousands of lines of code. (Even better than debug output and/or using a debugger is writing unit tests and testable code, but that's a little besides the point here).

 

That's also why I tried to get the code to run on my machine so I could add more debug output (of course you had it figured out on your own by then) :D. It's not always easy to follow a program's path just by looking at code, especially once threads get involved.

 

Writing readable code (and documenting it) is also a big help. Not only to others but also to yourself. I've cursed myself for writing code I couldn't figure out a month later more than once and I've also been greatful to my past self whenever I did write stuff that was easy to read and well documented.

[Eigenvektor]

Not trying to pick on you But I just noticed your code has a potential buffer overflow in it:

sPokemon galardex[1000];
f1.open("elenco.txt");

while(!f1.eof()){
	f1>>misc;
	galardex[i] = ...
	// [...]
	i++;
}

If your file contains more than 1000 lines you'll be attempting to access an index that doesn't exist. Either make sure you don't read more than the size of your array, or use some type of dynamically allocated array. It's probably harmless here, but that's how code vulnerable to buffer overflows is created.

 

For performance reasons its probably better to process the file line by line instead of reading all of it into memory all at once.

[Epimetheus]

44 minutes ago, Eigenvektor said:

Not trying to pick on you But I just noticed your code has a potential buffer overflow in it:

sPokemon galardex[1000];
f1.open("elenco.txt");

while(!f1.eof()){
	f1>>misc;
	galardex[i] = ...
	// [...]
	i++;
}

If your file contains more than 1000 lines you'll be attempting to access an index that doesn't exist. Either make sure you don't read more than the size of your array, or use some type of dynamically allocated array. It's probably harmless here, but that's how code vulnerable to buffer overflows is created.

 

For performance reasons its probably better to process the file line by line instead of reading all of it into memory all at once.

Yeah i know about the array, and i made sure to shrink It as much as possibile. The file is like 850 lines. I don't know about dynamic arrays but they sound interesting, is it like lists?

[Epimetheus]

1 hour ago, Eigenvektor said:

 

If your program doesn't work as expected, adding more output to see what it does (or using a debugger) is a great way to understand what it does and why it does it. It's usually a lot more effective than just looking at the code. Especially when your codebase grows to contain hundreds of thousands of lines of code. (Even better than debug output and/or using a debugger is writing unit tests and testable code, but that's a little besides the point here).

Yeah I should've made more debugging output, that's what I usually do. Only reason I didn't do It this time was because I thought the problem was beyond my comprehension, since it's basically my first time applying threads to something

[Epimetheus]

2 hours ago, Sauron said:

How did you prove it was looping? Again, if you had just posted your output this would have been solved in seconds. Instead we had to go through about a dozen posts of you trying to clarify and getting defensive when given advice on making your code better overall.

You sound really condescending,unlike @Eigenvektor, who actually didn't get angry at a noob without first asking normally. Try to get your wording better, if you don't want to sound aggressive, man. I proved It was looping since there's a cout for i and j in the for loops in the threads. Without mentioning the fact It kept outputting out stuff, when the variable reached the condition to stop the for statement, it just kept going resetting the variable

[Eigenvektor]

Just now, Epimetheus said:

Yeah i know about the array, and i made sure to shrink It as much as possibile. The file is like 850 lines. I don't know about dynamic arrays but they sound interesting, is it like lists?

What I meant is, if someone were to replace your file with a version that has, for example, 10,000 lines, it could potentially be used as an exploit. I'm not saying it's a real issue here, but let's say this is an app that runs with root privileges, then such a buffer overflow could potentially be used to gain root permissions. Just something too keep in mind

 

Yeah, I essentially meant something like std::vector, where the underlying array is resized dynamically as elements are added.

[Epimetheus]

2 hours ago, Eigenvektor said:

It's fine I don't really speak Italian but I can kind of figure it out.

 

What exactly is the program supposed to be doing? It seems like you're reading a file into an array and then attempting to find specific Pokemon to count their use?

I'm actually re writing with a cleaner and faster code a program I've made a couple months ago. The full program creates a full competitive Pokémon team automatically based on usage and type matching, It's given me some great teams so far

[Epimetheus]

6 minutes ago, Eigenvektor said:

What I meant is, if someone were to replace your file with a version that has, for example, 10,000 lines, it could potentially be used as an exploit. I'm not saying it's a real issue here, but let's say this is an app that runs with root privileges, then such a buffer overflow could potentially be used to gain root permissions. Just something too keep in mind

 

Yeah, I essentially meant something like std::vector, where the underlying array is resized dynamically as elements are added.

I don't get much of the exploit part but i'll keep that in mind, thank you

[Eigenvektor]

Just now, Epimetheus said:

I don't get much of the exploit part but i'll keep that in mind, thank you

Let me try to explain it a different way:

Right now, you create an array that is 1000 elements in size. Then you read a file line by line, and put each line into your array (kind of). If your file has more than 1000 lines, this means you'll be trying to write values to array[1000], array[1001], ... and so on. In most cases this will just crash your program.

 

This kind of error is known as a buffer overflow and can be used to get your code to execute an attackers code, if done correctly. In a program that is running with elevated privileges an attacker could potentially use such a bug to get the computer to run their code and gain more privileges for themselves.

 

This means your loop should stop when either the end of file has been reached or your index is the same value as the size of your array.

 

10 minutes ago, Epimetheus said:

I'm actually re writing with a cleaner and faster code a program I've made a couple months ago. The full program creates a full competitive Pokémon team automatically based on usage and type matching, It's given me some great teams so far

There's a better way to use threads to speed it up, but that's probably overkill here. As long as the code that is doing the calculations is fast enough, you'll mostly be limited by your disk's read speed more than anything else. Using threads isn't going to gain you much.

 

If the code is more complex, then using one thread to read the data and multiple threads to parse it can improve speed considerable without the need to read the whole file into memory at once. You'd have to look up the producer/consumer problem.

[Epimetheus]

9 minutes ago, Eigenvektor said:

Let me try to explain it a different way:

Right now, you create an array that is 1000 elements in size. Then you read a file line by line, and put each line into your array (kind of). If your file has more than 1000 lines, this means you'll be trying to write values to array[1000], array[1001], ... and so on. In most cases this will just crash your program.

 

This kind of error is known as a buffer overflow and can be used to get your code to execute an attackers code, if done correctly. In a program that is running with elevated privileges an attacker could potentially use such a bug to get the computer to run their code and gain more privileges for themselves.

 

This means your loop should stop when either the end of file has been reached or your index is the same value as the size of your array.

 

There's a better way to use threads to speed it up, but that's probably overkill here. As long as the code that is doing the calculations is fast enough, you'll mostly be limited by your disk's read speed more than anything else. Using threads isn't going to gain you much.

 

If the code is more complex, then using one thread to read the data and multiple threads to parse it can improve speed considerable without the need to read the whole file into memory at once. You'd have to look up the producer/consumer problem.

So like, i read from the file and then i use other threads to place what the main thread picked from the file in the array? 

[Eigenvektor]

2 minutes ago, Epimetheus said:

So like, i read from the file and then i use other threads to place what the main thread picked from the file in the array? 

Yes, the basic idea would be to have one thread that reads data from the file (e.g. line by line). It places that data into a queue with a fixed size. If the queue fills up, the thread waits until room is available again. You then have multiple threads that take data from that queue and process it (e.g. one thread per CPU). Those threads sleep as long as no further data is available. If the thread that fills the queue is fast enough there should be no "starvation" and the worker threads can work at a steady rate.

 

You also need to make sure there is some kind of signal that tells the worker threads that no further data is coming to allow them to shut down once all work has been processed. This is sometimes known as a "poison pill", because it terminates the worker threads.

 

This definitely requires synchronization to ensure that the thread writing data to the queue and the threads reading data do not come into conflict and to ensure the reading threads don't process the same work items. The worker threads would then put the results they produce either into a different queue or into an array (which requires locking again to avoid overwriting other thread's results).

 

Since the whole thing is complex enough without having to worry about arrays and indexes I would definitely use std::deque and std::vector over raw arrays and for example std::mutex to lock shared resources.