Thread: Function not changing variable value (I know about the pass-by-value mechanism)

So I have some code that is supposed to be a little exponents game, I have it so whenever you get a question wrong, it should make it the next person's (that has lives left) turn unless no one else has any lives, so I have a function, ret_turn(), that should be setting the variable "turn" to the appropriate value, I have it so it is given the address-of "turn" and the function is expecting a pointer but the value in turn never changes and a segmentation fault occurs, any help? I'll include more details if anyone wants. Sorry that I do not have any comments.

Code:

#include <stdio.h>
#include <math.h>
#include <time.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>




typedef struct Player
{
    char p_name[10];
    int lives;
    int points;
    int streak;
} player;


void ret_turn(int players, int *turn, player Player[], bool cont)
{    
    int previous_turn = *turn;
    
    do{
        *turn++;
        if(*turn > players)
        {
            *turn = 0;
            break;
        }
        if(*turn == previous_turn)
        {
            cont = false;
            break;
        }
    }while(Player[*turn].lives == 0);
    return;
}


int main(void)
{
    int exponent = 0;
    int rand_num = 0;
    double guess = 0;
    double answer = 0;
    int players = 0;
    int highest_score = 0;
    int winner = 0;
    player Player[10];
    bool cont = true;
    int turn = 0;


    srand(time(NULL));


    printf("How many people are playing?\n");
    scanf("%d", &players);
    if(players > 10 || players < 1)
    {
        printf("Invalid amount of players");
        return 1;
    }


    for(int i = 0; i < players; i++)
    {
        strcpy(Player[i].p_name, "0");
        Player[i].lives = 0;
        Player[i].streak = 0;
        Player[i].points = 0;
    }


    for(int i = 0; i < players; i++)
    {
        printf("Enter player %d's name: ", i+1);
        scanf("%s", Player[i].p_name);
    }


    printf("How many lives do you want to have? ");
    scanf(" %d", &Player[0].lives);
    if(Player[0].lives < 0)
    {
        printf("Invalid amount of lives");
        return 1;
    }
    for(int i = 0; i < players;i++)
    {
        Player[i].lives = Player[0].lives;
    }




    while(cont == true)
    {
        exponent = (rand()%5)-(rand()%10);
        rand_num = rand()%10;
        answer = pow(rand_num, exponent);


        printf("%s:\nWhat is %d to the power of %d?\n", Player[turn].p_name, rand_num, exponent);
        scanf(" %lf", &guess);


        if(answer == guess)
        {
            Player[turn].streak++;
            Player[turn].points = Player[turn].streak + 1;
            printf("\ncorrect!\n");
        }


        if(answer != guess)
        {
            Player[turn].streak = 0;
            Player[turn].lives--;
            printf("\nincorrect!\n");


            ret_turn(players, &turn, Player, cont);
        }

    }


    for(int i = 0; i < players; i++)
    {


    }
    return 0;
}

Originally Posted by anduril462

Here's what I get when I compile your code at maximum warning level:

Code:

$ make foo
gcc -Wall -ggdb3 -pedantic -std=gnu99 -O0 -o foo foo.c -lm -lpthread -lrt
foo.c: In function ‘ret_turn’:
foo.c:25:9: warning: value computed is not used [-Wunused-value]
         *turn++;
         ^
foo.c: In function ‘main’:
foo.c:49:9: warning: unused variable ‘winner’ [-Wunused-variable]
     int winner = 0;
         ^
foo.c:48:9: warning: unused variable ‘highest_score’ [-Wunused-variable]
     int highest_score = 0;
         ^

That first warning points you (indirectly) to the error. It's telling you that you never use (store, print, use in a different calculation), the result of *turn++.

Check your handy operator precedence chart. You'll notice that ++ (postfix increment) is higher precedence than * (dereference). That means it applies first. So your code takes the address stored in the turn pointer, increments it (points it to a difference piece of memory¹) and then dereferences (gets the value from) that new address. But you don't do anything with that value. What you really are trying to do is the reverse. Get the value stored at the address in turn, and increment that. Force the order you want with parens:

Code:

(*turn)++;

If you're ever unsure about stuff like this, there's no real harm in being a bit more explicit:

Code:

*turn += 1;
// or
*turn = *turn + 1;

Since your function is returning void, you can make it return int and return the turn number that way.

You have a few other issues with your code, even after fixing that:

1. Your code behaves as though there is one extra player; if I enter 2 players, it thinks there are 3.
2. Your code doesn't stop when all players are out of lives.
3. cont is set to false in ret_turn, but that value is not reflected in the main function, i.e. you pass cont by value, you should pass it by reference (same as you do turn).
4. Don't use magic numbers, define constants with sensible names: MAX_PLAYERS, MIN_PLAYERS, MAX_PLAYER_NAME, etc.
5. fgets is better for reading in player names, it allows you to specify a max length. scanf does let you do this, but it is less flexible, and more prone to writing past the end of your string¹.
6. Break your code up better into functions. Have one function that gets game parameters (num players, player names, num lives). Have another that determines if the game is over. Have one that gets the next player who is still alive. Give them sensible names like: get_game_parameters, is_game_over, get_next_player.
7. strcpy(Player[i].p_name, "0"); doesn't make a blank string, it gives every player the name "0" (a one-char name consisting of the digit zero). You probably want strcpy(Player[i].p_name, ""); or Player[i].p_name[0] = '\0'; to make them empty strings.

¹ This means you access memory you don't own. This results in undefined behavior, which is bad.

Okay then, that makes sense why it would have a seg fault. This is still a WIP so I still need to do some stuff (like break it up more, make it do stuff when everyone is out of lives (which is what that for loop is eventually going to do) etc.) Thanks, your post was really, really helpful.

Originally Posted by YayIguess

Okay then, that makes sense why it would have a seg fault. This is still a WIP so I still need to do some stuff (like break it up more, make it do stuff when everyone is out of lives etc.) Thanks, your post was really, really helpful.

Often it's a good idea to "break it up" in the first place.

E.g. you might have

Code:

#include <stdio.h>
#include <math.h>
#include <time.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>

#define STUB_FN printf("Implement me! Stub function called: %s()" \
    "File: %s Line %d\n", __func__, __FILE__, __LINE__);

typedef struct Player
{
    char p_name[10];
    int lives;
    int points;
    int streak;
} Player;

void init_player(Player *player)
{
    memset(player, 0, sizeof(*player));
}

int get_turn(Player *player)
{
    STUB_FN

    return 0; // FIXME
}

bool check_for_win(Player *player, int turn)
{
    STUB_FN

    return true; // FIXME
}

void print_result(Player *player, int turn, bool won)
{
    STUB_FN
}

int main(void)
{
    Player player;
    int turn;
    bool won = false;

    init_player(&player);
    
    do {
        turn = get_turn(&player);
        won = check_for_win(&player, turn);
        print_result(&player, turn, won);
    } while (won == false);

    return 0;
}

which might print:

Code:

Implement me! Stub function called: get_turn() File: t.c Line 27
Implement me! Stub function called: check_for_win() File: t.c Line 34
Implement me! Stub function called: print_result() File: t.c Line 41

This way you can get a general outline/framework for your program done and not really have done much at all (yay ) except get things clear in your mind (which means you've done something after all ). The program doesn't actually do anything at this point, of course.

I normally wouldn't write a program "upside down" (I'd use function prototypes), either.

> get_game_params(&Player[10], &num_of_players); //how function is being called
An array name will automatically decay into a pointer to the first element. Try:
get_game_params(Player, &num_of_players);

> main.c:104:26: error: request for member ‘p_name’ in something not a structure or union
Player is an array of structs, so you should access an element like any other array. The easiest way to access the field of some struct in an array is to use a different operator.
Player[i]->p_name[0] = '\0';
So a line like this actually accesses the first character of the field p_name in the structure referred to as Player[i]. The effect is to create an empty C string. Note that if you insist on using * to dereference you will have to place parens in the appropriate places. As written previously, * is also lower than dot (.) on the precedence table, which is used to access fields.