Why does my function keep looping in c++?

[Epimetheus]

#include <iostream>
#include <string>
#include <fstream>
#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <thread>

using namespace std;

struct sPokemon{
	string nome;
	string tipo;
	int speed;
	int util;
};

void lookname1(int n, sPokemon galardex[],string nome){
	int start=0;
	for(int i=start;i<n/2;i++){
		cout<<"i e' "<<i<<endl;

		if(nome==galardex[i].nome){
			galardex[i].util++;
			break;
		}
	}
}

void lookname2(int n, sPokemon galardex[],string nome){
	int start=n/2;
	for(int j=start;j<n;j++){
		cout<<"j e' "<<j<<endl;

		if(nome==galardex[j].nome){
			galardex[j].util++;
			break;
		}
	}
}

void inserisci(sPokemon galardex[],int n,string privileged_access){
	int i;
	string nome,fine;
	thread t1,t2;
	if(privileged_access==""){
		do{
			fflush(stdin);
			getline(cin,nome);
			t1= thread(lookname1,n,galardex,nome);
			t2= thread(lookname2,n,galardex,nome);
			t1.join();
			t2.join();
		}while(nome!="Fine");
	}
	else{
		nome=privileged_access;
		t1= thread(lookname1,n,galardex,nome);
		t2= thread(lookname2,n,galardex,nome);
		t1.join();
		t2.join();
	}
	
}

int main(int argc, char** argv) {
	
	fstream f1;
	int i,c=0;
	string misc;
	sPokemon galardex[1000];
	f1.open("elenco.txt");
	while(!f1.eof()){
		f1>>misc;
		if(c==0){
			galardex[i].nome=misc;
			c++;
		}
		else if(c==1){
			galardex[i].tipo=misc;
			c++;
		}
		else if(c==2){
			galardex[i].speed= atoi(misc.c_str());
			c++;
		}
		else if(c==3){
			galardex[i].util= atoi(misc.c_str());
			c=0;
			i++;
		}
	}
	int n=i;
	f1.close();
	
	int selett;
	do{
		cin>>selett;
		switch(selett){
			case 0:
				inserisci(galardex,i,"");
				break;
		}	
	}while(selett!=0);
	
	return 0;
}

So basically, i'm having a problem in the function inserisci, when i enter the privileged access if. It's supposed to let me enter a name,  the program looks for the same name in the list, and if it finds it it ups it count by 1. However, when it's done checking the list, it just loops endlessly, when it is supposed to ask me for another name. I've tried removing the threads, but it isn't working regardless. So what may be the problem?

[WereCatf]

8 minutes ago, Epimetheus said:

So what may be the problem?

You're comparing the fine-variable at 

while(fine!="Fine");

but you don't actually set it anywhere and thus the comparison is always true.

[C2dan88]

Your do while loop will loop indefinitely here 

	string nome,fine;
	thread t1,t2;
	if(privileged_access==""){
		do{
			fflush(stdin);
			getline(cin,nome);
			t1= thread(lookname1,n,galardex,nome);
			t2= thread(lookname2,n,galardex,nome);
			t1.join();
			t2.join();
		}while(fine!="Fine");

Because your never update the value for variable called fine. Causing your code to never exit the inserisci function and so it will not ask for another name.

[Epimetheus]

Just now, WereCatf said:

You're comparing the fine-variable at 

while(fine!="Fine");

but you don't actually set it anywhere and thus the comparison is always false.

It was just for debugging purposes, i edited the post rn

[Epimetheus]

Just now, C2dan88 said:

Your do while loop will loop indefinitely here 

	string nome,fine;
	thread t1,t2;
	if(privileged_access==""){
		do{
			fflush(stdin);
			getline(cin,nome);
			t1= thread(lookname1,n,galardex,nome);
			t2= thread(lookname2,n,galardex,nome);
			t1.join();
			t2.join();
		}while(fine!="Fine");

Because your never update the value for variable called fine. Causing your code to never exit the inserisci function and so it will not ask for another name.

re-read the code in the post

[Epimetheus]

it doesn't stop even when i put in "Fine" directly. It just never seems to reach the while statement

[Eigenvektor]

2 minutes ago, Epimetheus said:

it doesn't stop even when i put in "Fine" directly. It just never seems to reach the while statement

That suggests your thread.join hangs indefinitely, so you'd have to check whether your threads actually terminate.

[Sauron]

10 minutes ago, Epimetheus said:

it is supposed to ask me for another name

How do you know it's not "asking" for another name? You don't print anything to give the user that information at each cycle...

 

also,

• why do you have two separate "lookname" functions when you could just make a single function that takes the value of "start" as a parameter?
• what are you using threads for?
• without pointers you're not actually modifying your array in the functions, just a copy of it
• why are you using a do...while loop? if you insert "Fine" it will still cycle once before exiting which is not what you want
• a few comments couldn't hurt

5 minutes ago, Epimetheus said:

it doesn't stop even when i put in "Fine" directly. It just never seems to reach the while statement

Can you share any of your debugging efforts? How did you come to this conclusion when the program doesn't output anything? Did you use gdb or what?

[Epimetheus]

3 minutes ago, Eigenvektor said:

That suggests your thread.join hangs indefinitely, so you'd have to check whether your threads actually terminate.

why do they restart though? When i don't end them they just loop? Even so, i've written t1.join() afterwards, i should've ended it. The threads definitely exit the for loop, but then they just restart

[Eigenvektor]

You're also modifying the same array in two different threads without any kind of synchronization (e.g. mutex, semaphore, that kind of stuff), which is just asking for trouble.

[Epimetheus]

Just now, Eigenvektor said:

You're also modifying the same array in two different threads without any kind of synchronization (e.g. mutex, semaphore, that kind of stuff), which is just asking for trouble.

Well, the two threads handle each one half of the array, they should be fine

[Eigenvektor]

Just now, Epimetheus said:

Well, the two threads handle each one half of the array, they should be fine

No. Don't do that. Like ever. If you have a shared resource between two threads (three actually), synchronize access. You access the same resource in your main thread and two additional threads you're starting.

 

Threads can, for example, cache their own version of a variable for performance reasons unless you have some kind of memory barrier. This means changes done by one thread may or may not be visible to other threads.

[Epimetheus]

11 minutes ago, Sauron said:

How do you know it's not "asking" for another name? You don't print anything to give the user that information at each cycle...

 

also,

• why do you have two separate "lookname" functions when you could just make a single function that takes the value of "start" as a parameter?
• what are you using threads for?
• without pointers you're not actually modifying your array in the functions, just a copy of it
• why are you using a do...while loop? if you insert "Fine" it will still cycle once before exiting which is not what you want
• a few comments couldn't hurt

Can you share any of your debugging efforts? How did you come to this conclusion when the program doesn't output anything? Did you use gdb or what?

Well sorry, i'm used to writing the codes for myself, so I don't comment usually. Second, the two "lookname" functions isn't a problem, i was checking out if the surefire way would've worked. 

Third- are you sure? how do I modify the original array? I've already tried it a couple of times in different programs and it worked that way;

Fourth - idk, doesn't bother me that much.

 

Also, since i've coded it myself and i just copied this part from another program of mine where it worked, i'm sure asking for another name is how it's supposed to work.

If you want i can send the file to you so you can compile for yourself

[Epimetheus]

3 minutes ago, Eigenvektor said:

No. Don't do that. Like ever. If you have a shared resource between two threads (three actually), synchronize access. You access the same resource in your main thread and two additional threads you're starting.

 

Threads can, for example, cache their own version of a variable for performance reasons unless you have some kind of memory barrier. This means changes done by one thread may or may not be visible to other threads.

Is it just bad programming practice or it's just straight up not going to work? Because I think the array gets shared, and the main thread doesn't do anything while the other two threads work. I'm it this way mainly because I haven't studied threads fully yet, just the basics, and my professor doesn't look like he knows what he's doing - that's the main reason why I want to get ahead by myself

[Eigenvektor]

Just now, Epimetheus said:

Is it just bad programming practice or it's just straight up not going to work? Because I think the array gets shared, and the main thread doesn't do anything while the other two threads work. I'm it this way mainly because I haven't studied threads fully yet, just the basics, and my professor doesn't look like he knows what he's doing - that's the main reason why I want to get ahead by myself

It is bad practice and can lead to subtle bugs that are very hard to debug later on. Doing multi-threading correctly is not an easy topic. It's probably fine here, but like I said you can run into issues where one thread doesn't see updates to a shared variable because the compiler has decided to give each thread its own copy to improve performance since there is no memory barrier to ensure updates are visible to others. In some cases simply declaring a variable "volatile" can be enough, in others more elaborate locking schemes are needed.

[Epimetheus]

1 minute ago, Eigenvektor said:

It is bad practice and can lead to subtle bugs that are very hard to debug later on. Doing multi-threading correctly is not an easy topic. It's probably fine here, but like I said you can run into issues where one thread doesn't see updates to a shared variable because the compiler has decided to give each thread its own copy to improve performance since there is no memory barrier to ensure updates are visible to others. In some cases simply declaring a variable "volatile" can be enough, in others more elaborate locking schemes are needed.

I see. Well, i've already seen for myself multi-threading isn't an easy topic, lol. Mainly because my program doesn't work

[Epimetheus]

Well shit. I'm a fucking idiot. I kept inputting 1 in the menu switch statement, but I had to press 0. No clue why it outputted my entire list though.

[Eigenvektor]

2 minutes ago, Epimetheus said:

Well shit. I'm a fucking idiot. I kept inputting 1 in the menu switch statement, but I had to press 0. No clue why it outputted my entire list though.

So does it work now? I've tried to run it, but I just immediately get a segmentation fault I haven't done C++ in a while, so that might explain things

$ g++ test.cpp -o test -pthread
$ ./test

Segmentation fault (core dumped)

 

[Epimetheus]

Just now, Eigenvektor said:

So does it work now? I've tried to run it, but I just immediately get a segmentation fault I haven't done C++ in a while, so that might explain things

$ g++ test.cpp -o test -pthread
$ ./test

Segmentation fault (core dumped)

 

yeah it actually does pretty nicely, not sure if i've gained much performance but at least it works lol. Maybe it doesn't work since you don't have the file it needs to run? 

[Eigenvektor]

2 minutes ago, Epimetheus said:

yeah it actually does pretty nicely, not sure if i've gained much performance but at least it works lol. Maybe it doesn't work since you don't have the file it needs to run? 

Oh, right, I need to create an "elenco.txt"

[Epimetheus]

1 minute ago, Eigenvektor said:

Oh, right, I need to create an "elenco.txt"

i'm italian, so that's the reason why I've got some italian words in there lol

[Sauron]

19 minutes ago, Epimetheus said:

Well sorry, i'm used to writing the codes for myself, so I don't comment usually.

It doesn't matter who you're writing this for, 2 months from now you'll look at this and have no clue what any of it does.

20 minutes ago, Epimetheus said:

Third- are you sure? how do I modify the original array? I've already tried it a couple of times in different programs and it worked that way;

It's hard to say what will work given I don't know what you're trying to accomplish.

20 minutes ago, Epimetheus said:

Fourth - idk, doesn't bother me that much.

It doesn't bother you that the program has incorrect behavior that could be easily fixed by just using a normal loop?

23 minutes ago, Epimetheus said:

If you want i can send the file to you so you can compile for yourself

Yeah, why don't I rewrite the program for you, too, while I'm at it?

10 minutes ago, Epimetheus said:

Well shit. I'm a fucking idiot. I kept inputting 1 in the menu switch statement, but I had to press 0. No clue why it outputted my entire list though.

So yeah, we could have figured this out in 2 seconds had you posted any output at all or any of your debugging efforts. We can't read your mind, if you start asking questions properly you'll get a lot more useful help.

[Epimetheus]

1 minute ago, Sauron said:

It doesn't matter who you're writing this for, 2 months from now you'll look at this and have no clue what any of it does.

It's hard to say what will work given I don't know what you're trying to accomplish.

It doesn't bother you that the program has incorrect behavior that could be easily fixed by just using a normal loop?

Yeah, why don't I rewrite the program for you, too, while I'm at it?

So yeah, we could have figured this out in 2 seconds had you posted any output at all or any of your debugging efforts. We can't read your mind, if you start asking questions properly you'll get a lot more useful help.

jesus christ man why are you being this rude. It doesn't have "incorrect behavior", it just does another scan of the array without doing anything. Pretty painless, isn't it?

"Why don't i rewrite the program for you while i'm at it"? Is copying and pasting code a difficult thing to do?

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

[Unimportant]

Flushing stdin is undefined behavior. Use std::istream::ignore to clear input streams.

cin.ignore(std::numeric_limits<std::streamsize>::max())

 

[Eigenvektor]

11 minutes ago, Epimetheus said:

i'm italian, so that's the reason why I've got some italian words in there lol

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?