22

I've been trying to wrap my head around this the whole day...

Basically, I have a struct called State that has a name and another one called StateMachine with a name, an array of states and total number of states added:

#include <stdio.h>
#include <stdlib.h>

typedef struct State {
  const char * name;

} State;

typedef struct StateMachine {
  const char * name;

  int total_states;
  State ** states;

} StateMachine;

StateMachine * create_state_machine(const char* name) {
  StateMachine * temp;

  temp = malloc(sizeof(struct StateMachine));

  if (temp == NULL) {
    exit(127);
  }

  temp->name = name;
  temp->total_states = 0;

  temp->states = malloc(sizeof(struct State));
  return temp;
}

void destroy_state_machine(StateMachine* state_machine) {
  free(state_machine);
}

State * add_state(StateMachine* state_machine, const char* name) {
  State * temp;

  temp = malloc(sizeof(struct State));

  if (temp == NULL) {
    exit(127);
  }

  temp->name = name;

  state_machine->states[state_machine->total_states]= temp;
  state_machine->total_states++;

  return temp;
}

int main(int argc, char **argv) {

  StateMachine * state_machine;

  State * init;
  State * foo;
  State * bar;

  state_machine = create_state_machine("My State Machine");

  init = add_state(state_machine, "Init");
  foo  = add_state(state_machine, "Foo");
  bar  = add_state(state_machine, "Bar");

  int i = 0;

  for(i; i< state_machine->total_states; i++) {
    printf("--> [%d] state: %s\n", i, state_machine->states[i]->name);
  }

}

For some reason (read low C-fu / years of ruby/python/php) I'm unable to express the fact that states is an Array of State(s). The above code prints:

--> [0] state: ~
--> [1] state: Foo
--> [2] state: Bar

What happened with the first state added?

If I malloc the states array on the first state added (e.g. state_machine = malloc(sizeof(temp)); then I get the first value but not the second.

Any advices?

This is a C question. I'm using gcc 4.2.1 to compile the sample.

Improve this question

5 Answers 5

Reset to default
13

It looks like you're not allocating space for your states in the machine past the first one.

StateMachine * create_state_machine(const char* name) {
  StateMachine * temp;

  temp = malloc(sizeof(struct StateMachine));

  if (temp == NULL) {
    exit(127);
  }

  temp->name = name;
  temp->total_states = 0;

  temp->states = malloc(sizeof(struct State)); // This bit here only allocates space for 1.
  return temp;
}

You're probably better off putting an array of states of fixed size in the state machine struct. If that's not okay, you'll have to realloc and move the whole set around or allocate chunks and keep track of the current length, or make a linked list.

Incidentally, init, foo, and bar never get used.

Edit: What I'm suggesting looks like this:

#define MAX_STATES 128 // Pick something sensible.
typedef struct StateMachine {
  const char * name;
  int total_states;
  State *states[MAX_STATES];
} StateMachine;
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

If I set that to NULL (because I won't know the size of the states when creating the StateMachine) how will I add the states?
@apann: You don't need to know the sizes of the states, only the size of a pointer to a state. Sticking an array of State * in StateMachine should work, as long as you make sure not to overrun it.
Thanks, I would have up voted this but I don't have enough karma :)
@apann: Keep your question quality where it is and you'll have no trouble with rep. This is one of the best first questions I've seen.
12

It looks like you want to have a variable number of states in each state machine, but you are allocating the memory incorrectly. In create_state_machine, this line:

temp->states = malloc(sizeof(struct State));

Allocates a single State object, not an array of pointers (which is how you are using it).

There are two ways you could change this.

  1. Declare states as State states[<some-fixed-size>]; but then you cant ever have more than a fixed number of states.
  2. Add another member to indicate how much storage has been allocated for states, so you can keep track of that as well as how much is used (which is what total_states is being used for).

The later would look something like this:

#include <stdlib.h>
#include <string.h>

typedef struct 
{
    const char *name;
} State;

typedef struct 
{
    const char *name;
    int total_states;
    int states_capacity;
    State *states;
} StateMachine;

StateMachine *create_state_machine(const char *name)
{
    StateMachine *temp = malloc(sizeof(StateMachine));
    memset(temp, 0, sizeof(*temp));

    temp->name = name;
    temp->states_capacity = 10;
    temp->states = malloc(sizeof(State) * temp->states_capacity);

    return temp;
}

State *add_state(StateMachine *machine, const char *name)
{
    if (machine->total_states == machine->states_capacity)
    {
        // could grow in any fashion.  here i double the size, could leave
        // half the memory wasted though.
        machine->states_capacity *= 2;

        machine->states = realloc(
            machine->states, 
            sizeof(State) * machine->states_capacity);
    }

    State *state = (machine->states + machine->total_states);
    state->name = name;

    machine->total_states++;

    return state;
}
Improve this answer

1 Comment

Thanks! I see this working, but I can't pick more that 1 answer :)
4

Inside of your add_state function:

temp = malloc(sizeof(struct StateMachine));

should be 

temp = malloc(sizeof(struct State));

However, even when this is changed, I still get the proper output:

--> [0] state: Init
--> [1] state: Foo
--> [2] state: Bar

Perhaps there's nothing wrong with your code. I'm using gcc version 4.4.3

Improve this answer

2 Comments

Thanks for spotting this, I'll edit my post. However, it didn't fix the problem (I'm getting same result)
I wonder if there's a compiler flag or something that will make this work under 4.2.1 (default compiler under Snow Leopard).
1 
State ** states;

will create an array of state arrays.

I haven't read through the whole solution truthfully (gotta run), but you mentioned wanting an array of states - did you possibly want to do:

State* states

or

State states[size];

instead? Just food for thought, chances are it wasn't your problem since I didn't fully read it :p

Improve this answer

1 Comment

I've tried that as well, but I'm getting error: incompatible types in assignment when doing state_machine->states[state_machine->total_states]= temp;
1

You're doing a conceptual error:

State ** states;

It's true that you can consider states like an array of pointer to State object, but you are allocating space for just one state. When you do:

state_machine->states[state_machine->total_states]= temp;

you are doing something wrong if total_states is greater than zero because you are pointing to memory segments that are not been allocated (I'm wondering why you don't get a SEGFAULT). To store a dynamic number of State this way you need a linked list, or to call realloc every state you add(but that's not a good idea). The memory you are allocating with different malloc calls isn't continuous.

Improve this answer

2 Comments

The segfault is probably because states are very small (only 1 word each). Were C a safer language, it would check this sort of thing.
Thanks for the details, now I understand what I was doing wrong.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.