Determine array size at runtime to read in user input

[bazingatheory]

hi i am really new to this and can't see why i can't get my code to work, it is probily really silly but help would be appreciated.
i am trying to get the user to enter the number of competators of a 4 lap race so that it can determine the lap size then read in the lap times of each competator.

# include <stdio.h>
# include <malloc.h>

/* define the structure , what you want the user to input*/
    struct laptimes {
        float lap1;
        float lap2;
        float lap3;
        float lap4;
    };

    int main (){
        int comp ;
        int n;
        printf("Enter number of competators \n");
        scanf("%d",&n);
        comp = calloc (n,sizeof (int));

        /* allocating the pointer to the memory space*/
    int p;
    struct laptimes * ptr = &p;

    p = malloc (sizeof(int));

    /* asking the user to input lap times. Scan in the user input then print it back out to the user*/
    printf("Enter  lap times \n ");
    scanf("%f%f%f%f",(*ptr).lap1,(*ptr).lap2,(*ptr).lap3,(*ptr).lap4);
    printf("%f%f%f%f",(*ptr).lap1,(*ptr).lap2,(*ptr).lap3,(*ptr).lap4);

    free();
    return 0 ; 
}

[Banfa]

free has to be called with the pointer you wish to free so the syntax at line 30 is wrong.

comp is never used you can remove lines 13 and 17 without affecting your program.

line2 20, 21 and 23 really make no sense. What you want to do is allocate an array of struct laptimes to hold you results. What you do is point you laptimes pointer ptr to the address of an integer, then you allocate memory and assign it's address to the integer.

line 21 ptr = address of p
line 23 p = address of memory block that is the size of an int.

replace lines 20 - 23 with

struct laptimes * ptr = malloc(n * sizeof(*ptr));

replace line 30 with

free(ptr);

Finally at lines 27 and 28 this (*ptr).lap1 is a slightly strange construct (although not wrong) however most people would use ptr->lap1, once ptr is allocated an array you may wish to use ptr[0].lap1 to get the idea of an array across.

[bazingatheory]

Thank you.
Now i am getting an error on line 18 parse error before `int' ?

[Moschops]

Given that we can't see your new line 18 or the code around it, you're going to have to show us the code again.

[bazingatheory]

# include <stdio.h>
# include <malloc.h>

/* define the structure , what you want the user to input*/
    struct laptimes {
        float lap1;
        float lap2;
        float lap3;
        float lap4;
    };

    int main (){
        int n;
        printf("Enter number of competators \n");
        scanf("%d",&n);

        /* allocating the pointer to the memory space*/
    int *ptr = n;
    struct laptimes * ptr = malloc(n*sizeof(*ptr));



    /* asking the user to input lap times. Scan in the user input then print it back out to the user*/
    printf("Enter  lap times \n ");
    scanf("%f%f%f%f",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
    printf("%f%f%f%f",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);

    free(ptr);
    return 0 ; 
}

[JasonHippy]

You're doing your memory allocation completely incorrectly. You need to allocate a block of memory for n instances of the laptimes structure. So perhaps declare two pointers to laptimes at the top of main:

int n, index;
laptimes* lapdata=0;
laptimes* competitor=0;

The first pointer (lapdata) will be assigned to point to the block of memory allocated to hold the lap times for all competitors. The second pointer (competitor) will be used to iterate through the block of memory to access each individual competitors times.

After the user has entered the number of competitors (n), you can allocate the memory for the lapdata like this:

lapdata = (laptimes*) calloc(n, sizeof(laptimes));

This will allocate space for n instances of the laptimes struct and assign lapdata to point to the block of memory that was allocated.
NOTE: calloc returns a void pointer (void*) which has to be cast to a pointer of the appropriate type - in this case we need to cast it to a laptimes pointer (laptimes*)

Next you need to ensure that the lapdata pointer is valid (i.e. not NULL). As long as lapdata is valid, set the pointer called competitor to point to lapdata (making it point to the first competitors data) before using a loop to allow the user to enter each competitors laptimes.

if(lapdata)
{
    /* set competitor to point to first competitors times*/
    competitor = lapdata;

    /* fill lapdata with each competitors times */
    for(index=0; index<n; ++index, ++competitor)
    {
        /* code to enter lapdata for each competitor to go here 
           I'll leave you to work this bit out */
    }
}

Finally at the end of your program, you must remember to call free on lapdata to free/deallocate memory!

I think I've given you more than enough to go on here. You should be able to work out the rest!

EDIT: Sorry to seem like I'm butting in! I got called to a meeting while I was in the middle of composing this response an hour or so ago. When I returned I forgot to check if there were any other responses before submitting! Sry! :)

[Moschops]

int *ptr = n;

n is an int. You are trying to set an int-pointer to have the same value as an int. This makes no sense. No sense at all. Do you understand what a pointer is?

[Banfa]

Line 18 is just wrong, I didn't say to put in anything like that and I can't tell why you think it is required but you can just remove it.

[Banfa]

NOTE: calloc returns a void pointer (void) which has to be cast to a pointer of the appropriate type - in this case we need to cast it to a laptimes pointer (laptimes)

Very wrong, calloc does return void* but you do not need to cast it if you are using C, in fact casting void* in C is poor practice the compiler knows what to do and you should let it. By casting you are telling the compiler you know better than it and to just do what you say without any sanity check but actually you don't know better than the compiler.

If you are going to reply to say that you get an error if you don't put the cast in then you are almost certainly using a C++ compiler which does not automatically convert void*. In this case you shouldn't be using malloc or calloc any but rather new since you are using C++.

[JasonHippy]

It was definitely C code I posted, not C++. And I didn't mention anything about errors, I didn't compile anything, I just made a few code suggestions. ;)

However, it's been a long time since I used C regularly and I think perhaps the standards may have changed from when I originally learnt C back in the mid 90's (on an outdated version of Borlands compiler!)

Rightly or wrongly, I was taught to always cast the void* returned to the required type when using malloc and calloc. Although....Thinking back, I think it might have been an early version of Turbo C++ that we were using as a compiler. So if what you say is true, that could go some way towards explaining why we were told to cast... Hmmm, it was a long time ago though! :/ Was it Turbo C? or Turbo C++? I can't remember!

Now I'm questioning everything I've ever been taught! Heh heh! XD
OK, thanks for that Banfa, I'll bear that in mind!

[Banfa]

Now I feel I should have explained more (especially having remembered what the issue is).

The problem is that in C you do not need to have a prototype for a function before you call it. That means you don't have to include stdlib.h before calling malloc/calloc. Without the inclusion the compiler assumes malloc/calloc return int.

If you don't have the cast then you get a warning at least however if you have the case you get no warning. This is bad on a 64 bit system because on most 64 bit systems today int is 32 bits and pointers are 64 bits. With the hidden implicit return type the 64 bit void * returned by malloc is converted to a 32 bit int and then converted to a 64 bit pointer, you corrupt the pointer value by loosing the top 32 bits.

Try compiling this code (as is i.e. no headers)

int main()
{
  long* pl1 = malloc(sizeof(long)*10);
  long* pl2 = (long*)malloc(sizeof(long)*10);

  return 0;
}

You only get warnings at line 3, none at line 4 the cast hides them.

That is why you shouldn't cast, to cover the case of you forgot to include the correct header.

[bazingatheory]

Sorry im confused so i should do this ?
I know im making a lot of mistakes but this is my first ever c program and it makes no sense to me sorry im trying to make sense of it all. i have only ever coded in vb.

/* define the structure , what you want the user to input*/
    struct laptimes {
        float lap1;
        float lap2;
        float lap3;
        float lap4;
    };

    int main (){
        int n;
        printf("Enter number of competators \n");
        scanf("%d",&n);


        /* allocating the pointer to the memory space*/
    int p;
     struct laptimes *ptr = malloc(n * sizeof(*ptr));

    /* asking the user to input lap times. Scan in the user input then print it back out to the user*/
    printf("Enter  lap times \n ");
    scanf("%f%f%f%f",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
    printf("%f%f%f%f",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);

    free(ptr);
    return 0 ; 
        }
    }

[Banfa]

It is not looking too bad.

Line 16: you don't use p and there is no need to for this line

Line 21: You need to provide pointers to scanf because it needs locations in which to store values not the values themselves, this should be written scanf("%f%f%f%f",&ptr->lap1,&ptr->lap2,&ptr->lap3,&ptr->lap4);

Line 22: Some space between your output values will make them easier to read

Line 27: You have 1 more closing brace than opening brace, just delete this one

Finally I'm not sure if you are using the C90 standard or the C99 standard. In the former case then you can not declare variables halfway through a code block as you have done at lines 16 and 17, in the latter case you can.

[bazingatheory]

ok i have this code now

# include <stdio.h>
# include <malloc.h>

/* define the structure , what you want the user to input*/
    struct laptimes {
        float lap1;
        float lap2;
        float lap3;
        float lap4;
    };

    int main (){
     int i,n;
     //int*ptr;
     struct laptimes *ptr = malloc(n * sizeof(*ptr));
        printf("Enter number of competators \n");
        scanf("%d",&n);


         for (i=0; i<n; i++)
         {

    /* asking the user to input lap times. Scan in the user input then print it back out to the user*/
    printf("Enter  lap times \n ");
    scanf("%d%d%d%d",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
    printf("%d%d%d%d",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
         }
    free(ptr);
    return 0 ; 

    }

which now runs but the console keeps getting shut down from and error ?

[Banfa]

Your problem is line 15, you use n to get the size of memory to allocate but at this point it is uninitialised (using, reading the value of, an uninitialised automatic variable is undefined behaviour) because you do not get the value from the user until line 17.

You need to split line 15 into 2 lines

struct laptimes *ptr = NULL; // Note always initialising pointers is good practice

ptr = malloc(n * sizeof(*ptr));

The first of those 2 should still be at line 15 and the second at line 18 so that you declare your pointer at the top of the function but allocate the memory for it after line 17 where the value of n is input by the user.

Also it is generally best practice after calling malloc to check that the pointer is not NULL before using it:

ptr = malloc(n * sizeof(*ptr));

if (ptr != NULL)
{
    // for loop scanf, printf etc

    free(ptr);
}

[bazingatheory]

thank you. i now have

# include <stdio.h>
# include <malloc.h>

/* define the structure , what you want the user to input*/
    struct laptimes {
        float lap1;
        float lap2;
        float lap3;
        float lap4;
    };

    int main (){
     int i,n;
     //int*ptr;
     struct laptimes *ptr = NULL;
      ptr=malloc(n * sizeof(*ptr));
        printf("Enter number of competators \n");
        scanf("%d",&n);

       if (ptr!=NULL);
         for (i=0; i<n; i++)
         {

    /* asking the user to input lap times. Scan in the user input then print it back out to the user*/
    printf("Enter  lap times \n ");
    scanf("%d%d%d%d",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
    printf("%d%d%d%d",ptr->lap1,ptr->lap2,ptr->lap3,ptr->lap4);
         }
    free(ptr);
    return 0 ; 

    }

but the console keeps shutting down is it my code or something else ?

[Moschops]

You didn't fix the problem with n.

Here is what you are doing:

Creating a variable, n. What value does it have? Nobody knows. Whatever random value is in the memory. Could be zero. Could be a billion.

You are then using n in a call to malloc. So how much memory are you getting from malloc? Nobody knows. Whatever random value is in the memory. Could be zero. Could be a billion.

You are then setting the value of n, using scanf.

Do you see the problem here?

[bazingatheory]

ahhh ok i wanted to set how many times it will loop based on what the user enters with the scanf but im using it with the malloc before the scanf

[bazingatheory]

so changing that around i can enter lap times then is stops working and shuts down

[Moschops]

You're using scanf wrongly. The parameters are meant to be pointers to the variable you want to store the data in.

Also, your malloc is clumsy. You want to store laptimes, so just get the size of a laptime.

[bazingatheory]

i don't have a size for laptimes its user defined

[Moschops]

i don't have a size for laptimes its user defined

struct laptimes {
float lap1;
float lap2;
float lap3;
float lap4;
};

Doesn't look user-defined to me. It's a struct of fixed size.

ptr=malloc(n * sizeof(struct laptimes));

laptimes is a struct contains float objects. You are storing them and reading them as ints. This is very bad.