Errors in my C program!! Pls help!!!

[Newbie_Gamerz]

So I have a code which calculates the class average, boys highest mark, girls highest mark, lowest mark etc.

 

When I enter the number of boys value below 10, I can enter any number of girls in the class and calculate many marks as I need (As seen in image) 

 

Now the problem is when I enter number of boys value as 10 or above I can't enter number of girls value greater than 10... It simply crashes and shoes a message as shown in the image.

And I have another problem in my code where I can successfully display the index who got the highest mark in both boys and girls but I can't display the index who got the lowest mark in both boys and girls... If I try to to it shows "The lowest mark in [boys/girls]* is [boy/girl]* 0 with 0 marks"       *Statements are separate

Here is my code (THIS IS C PROGRAM)

 

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

int bhigh(int b[], int c) //*Finding highest mark in boys*//
{ 
    int i, index = 0;

  for (i = 1; i <= c; i++)
    if (b[i] > b[index])
      index = i;

  return index; 
} 

int blow(int b[], int c) //*Finding lowest mark in boys*//
{ 
    int i; 
    int min = b[1]; 
  
    for (i = 1; i <= c; i++) 
        if (b[i] < min) 
            min = b[i]; 
  
    return min; 
} 

int ghigh(int g[], int d) //*Finding highest mark in girls*//
{ 
    int i, index = 0;

  for (i = 1; i <= d; i++)
    if (g[i] > g[index])
      index = i;

  return index; 
}  

int glow(int g[], int d) //*Finding lowest mark in girls*//
{ 
    int i; 
    int min = g[1]; 
  
    for (i = 1; i <= d; i++) 
        if (g[i] < min) 
            min = g[i]; 
  
    return min; 
}

int main()
{
    int i,c,d;
    int *b,*g;
    int bhighlocation, bmaximum, blowlocation, bminimum;
    int ghighlocation, gmaximum, glowlocation, gminimum;
    float sum1=0,sum2=0,avg1,avg2,clsavg;

    printf("C PROGRAM FOR A CLASS MARK LIST FOR BOYS AND GIRLS\n\n");

    //*Getting no. of boys*//
    printf("Enter the number of boys: ");
    scanf("%d",&c);
    
    b=(int*)malloc(c*sizeof(int));
    
    //*Getting boys marks*//
    printf("\nEnter the marks of boys :\n");
    for(i=1;i<=c;i++)
    {
        printf("    -Enter marks of boys %d: ",i);
        scanf("%d",&b[+i]);
    }
    
    //*Getting no. of girls*//
    printf("\nEnter the number of girls: ");
    scanf("%d",&d);
   
    g =(int*)malloc(d*sizeof(int));
    
    //*Getting girls marks*//
    printf("\nEnter the marks of girls :\n");
    for(i=1;i<=d;i++)
    {
        printf("    -Enter mark of girl %d: ",i);
        scanf("%d",&g[+i]);
    }
    
    //*Computing average marks of boys*//
    for(i=1;i<=c;i++)
    {
    sum1 = sum1+b[i];
    }
    avg1=sum1/c;
    printf("\nThe average of boys marks is : %.2f\n",avg1);
    
    //*Computing average marks of girls*//
    for(i=1;i<=d;i++)
    {
    sum2 = sum2+g[i];
    }
    avg2=sum2/d;
    printf("The average of girls marks is : %.2f\n",avg2);
    
    //*Computing overall class average*//
    clsavg=(avg1+avg2)/2;
    printf("The overall class average is : %.2f\n",clsavg);
    
    //*Printing highest mark between boys*//
    bhighlocation = bhigh(b, c);
    bmaximum  = b[bhighlocation];
    printf("\nHighest mark in boys is boy %d with %d marks.\n", bhighlocation, bmaximum);
    
    //*Printing lowest mark between boys*//
    printf("Lowest mark in boys is %d\n", blow(b, c));
    
    //*Printing highest mark between girls*//
    ghighlocation = ghigh(g, d);
    gmaximum  = g[ghighlocation];
    printf("\nHighest mark in girls is girl %d with %d marks.\n", ghighlocation, gmaximum);
    
    //*Printing lowest mark between girls*//
    printf("Lowest mark in girls is %d\n", glow(g, d));
    
    return 0;
}

Sorry if my code's too large.....

[SpaceGhostC2C]

Well, I don't see that getting triggered by the size of either b or g, but you are initializing index as zero? So the first comparison in your highest score loops compares the first element to "element 0" of each vector? 

 

With respect to the error itself, I'm not as familiar with C, although it somewhat resembles the messages I would sometimes get in Fortran dealing with assumed types/sizes. I'm afraid I can't be of much help. 

[Newbie_Gamerz]

3 hours ago, SpaceGhostC2C said:

Well, I don't see that getting triggered by the size of either b or g, but you are initializing index as zero? So the first comparison in your highest score loops compares the first element to "element 0" of each vector? 

 

With respect to the error itself, I'm not as familiar with C, although it somewhat resembles the messages I would sometimes get in Fortran dealing with assumed types/sizes. I'm afraid I can't be of much help. 

As per the question given by my teacher the index value starts from zero then two then so on.... That's why I initialized from zero... 

[shadow_ray]

Please try to choose more descriptive names, glow and blow are probably the most confusing:

int blow(int b[], int c) //*Finding lowest mark in boys*//

int glow(int g[], int d) //*Finding lowest mark in girls*//

 

Why would you write 2 functions that do the same thing? Both of these gonna find the smallest value in the array. Same thing applies to the other two.

 

edit: @suriya2210 consider using one of these: snake_case,  CamelCase

 

[Dat Guy]

3 hours ago, shadow_ray said:

glow and blow are probably the most confusing

I actually like those. 

[Newbie_Gamerz]

22 hours ago, shadow_ray said:

Please try to choose more descriptive names, glow and blow are probably the most confusing:

int blow(int b[], int c) //*Finding lowest mark in boys*//

int glow(int g[], int d) //*Finding lowest mark in girls*//

 

Why would you write 2 functions that do the same thing? Both of these gonna find the smallest value in the array. Same thing applies to the other two.

 

edit: @suriya2210 consider using one of these: snake_case,  CamelCase

 

@shadow_ray @Dat Guy I know they sound funny XD but for the sake of keeping the names short I kept 'glow' and 'blow'

Functionally they do the same thing but collect and store data to different places

 

[shadow_ray]

8 hours ago, suriya2210 said:

Functionally they do the same think but collect and store data to different places

 

WHAT? What does that matter?

 

I've spotted a few mistakes in your code:

for (i = 1; i <= c; i++)

Now you have to fix this in a few different places. That's what you get for duplicating code.

 

I would do something like this:

Spoiler

#include <stdio.h>

int max_idx(int arr[], int size){
  
    int index = 0;
    for (int i = 1; i < size; ++i)
        if (arr[i] > arr[index])
            index = i;

    return index; 
} 

int min_idx(int arr[], int size){
  
    int index = 0;
    for (int i = 1; i < size; ++i)
        if (arr[i] < arr[index])
            index = i;

    return index;
}

double avg(int arr[], int size){
    double accum = 0;
    for(int i = 0; i < size; ++i)
        accum += arr[i];
        
    return accum / size;
}

int main() {
  
    int arr[] = {1, 2, 10, 3, 1, 0};
    int arr_size = sizeof(arr)/sizeof(int);
    
    int idx = max_idx(arr, arr_size);
    printf("MAX index: %d, value: %d\n", idx, arr[idx]);
    
    idx = min_idx(arr, arr_size);
    printf("MIN index: %d, value: %d\n", idx, arr[idx]);
    
    printf("avg: %lf", avg(arr, arr_size));
    return 0;
}

 

 

The only difference between the min and max function is a single comparison. We could use a function to do the comparison for us by passing a function pointer to the original function:

Spoiler

#include <stdio.h>
#include <stdbool.h>

int find_idx(int arr[], int size, bool (*cmp)(int, int)){    
    int index = 0;
    for (int i = 1; i < size; ++i)
        if( cmp(arr[i], arr[index]))
            index = i;

    return index; 
} 

double avg(int arr[], int size){    
    double accum = 0;
    for(int i = 0; i < size; ++i)
        accum += arr[i];
        
    return accum / size;
}

bool greater(int a, int b){
    return a > b;
}

bool smaller(int a, int b){
    return a < b;
}

int main() {
    int arr[] = {1, 2, 10, 3, 1, 0};
    int arr_size = sizeof(arr)/sizeof(int);
    
    int idx = find_idx(arr, arr_size, greater);
    printf("MAX, index: %d, value: %d\n", idx, arr[idx]);
    
    idx = find_idx(arr, arr_size, smaller);
    printf("MIN, index: %d, value: %d\n", idx, arr[idx]);
    
    printf("avg: %lf", avg(arr, arr_size));
    return 0;
}

 

We could even use void* pointers so it could work with types other than int.

[shadow_ray]

Now I'm getting out of control This is a general solution, it should work with any type

#include <stdio.h>
#include <stdbool.h>
#include <math.h>

int find_idx(const void* arr, int arr_size, int type_size, bool (*cmp)(const void*, const void*)){    
    int index = 0;
    for (int i = 1; i < arr_size; ++i){
        const void* a_ptr = (char*)arr + type_size*i; // &arr[i]
        const void* b_ptr = (char*)arr + type_size*index; // &arr[index]
        if( cmp(a_ptr, b_ptr))
            index = i;
    }
    return index; 
} 

bool greater(const void* a, const void* b){
    return *((int*)a) > *((int*)b);
}

int main() {
    int arr[] = {1, 2, 10, 3, 1, 0};
    int arr_size = sizeof(arr)/sizeof(int);
	
    int idx = find_idx(arr, arr_size, sizeof(int), greater);
    printf("MAX, index: %d, value: %d\n", idx, arr[idx]);
}

 

smallest complex number based on absolute value

typedef struct {
    double i, r;
} complex;

bool smaller_complex(const void* a, const void* b){
    const complex* c_a = (complex*) a;
    const complex* c_b = (complex*) b;
    
    double abs_a = sqrt(pow(c_a->i, 2) + pow(c_a->r, 2));
    double abs_b = sqrt(pow(c_b->i, 2) + pow(c_b->r, 2));
    
    return abs_a < abs_b;
}

//main
complex c_arr[] = {{-1,0}, {0.25,0.1}, {1,2.5}};
int c_arr_size = sizeof(c_arr) / sizeof(complex);

int idx = find_idx(c_arr, c_arr_size, sizeof(complex), smaller_complex);
printf("MIN, index: %d, value: %lf,%lf\n", idx, c_arr[idx].i, c_arr[idx].r);

feedback is welcome

[wanderingfool2]

On 12/30/2020 at 2:49 AM, suriya2210 said:

@shadow_ray @Dat Guy I know they sound funny XD but for the sake of keeping the names short I kept 'glow' and 'blow'

Functionally they do the same thing but collect and store data to different places

 

I know shadow_ray sort of answered your question (and formed a generalized solution)...but thought that I would just chime in to explain why your code is crashing.  (Since it wasn't overly clear by the other's posts)

int bhigh(int b[], int c) //*Finding highest mark in boys*//
{ 
    int i, index = 0;

//int b[], has c units...that means you can do b[0], b[1], ..., b[c-1]...since array's start at 0 it can only go to c-1.

  for (i = 1; i <= c; i++) //You have i <= c.  This in effect means you do b[c] on the last loop...which is not a good thing
    if (b[i] > b[index])
      index = i;

  return index; 
} 

You are effectively reading in memory that is outside of the array you defined....this causes undefined behavior as you really don't know what is beyond.

You have a similar logic issue (of reading memory that is beyond the array) in the high and low functions for boys and girls.  (Which admittedly, you should just collapse into high and low...and not bother with boy or girl, as it's the same code)

 

Now for the bit that is likely to cause a crash

    for(i=1;i<=c;i++) //Same issue, b[c] is not part of the b array.  You want to go from 0 to i < c
    {
        printf("    -Enter marks of boys %d: ",i);
        scanf("%d",&b[+i]); //And here is the crashy part.  when b[c] you are writing to it...but b[c] again is past the array
    }
    //If you are lucky, there will be just junk non-important information there, and your program won't crash...but you should never rely on luck as writing to a part of memory you haven't defined is dangerous (in  that it could allow exploits) but most often just means unpredictable behavior.

The reason why 9 works and 10 doesn't...likely just luck of where the memory is...because when you set boys to 10 and you write b[10] = 10; you are actually writing to a section of memory that you don't "own" (for lack of a better non-technical descriptor).

 

 

So yea, the tl;dr is array's in C programming language go from 0 - [c-1].  You are consistently doing 1 - c (and in the case of high you are doing 0 - c).  If you correctly it so you go from 0 - [c-1] then I think you will find your code behaves closer to what you want

 

[wanderingfool2]

On 12/30/2020 at 9:45 AM, shadow_ray said:

feedback is welcome

So, I'm a bit bored and tired...so thought I'd give a bit of feedback.

 

Generalized functions can be useful to an extent, but the thing to remember as well it does come with a performance cost.  (Alas, that is always the trade-off isn't it).  So it would be important to realize where said function may be run (and how often).  Mainly because of the use of a functor, it means for each comparison a function has to be called...(so each element in the array would be 1 extra function and at least one extra register set...but likely more as it's sending the pointer as a parameter to the functor and the return value...less things that the compile time optimizer can do)

 

e.g. Imagine doing it on a 10 million int data set.  Your generic call would be take longer than a high/low function (totally get that one could also optimize the high/low code to be super efficient).

 

It's also the old school in me (and the fact that I saw someone lose out on a job opportunity), but your code isn't ANSI C complaint (not that many people use that now and days).  bool type (it was added in C99, but in Ansi C it doesn't exist)...but most modern places don't use ANSI C anymore...just my rambling.

 

The only real note I would make though that might be a bit of a nit-picky, but naming the greater function as greater...instead of something like greater_int (to specify that it will be casting ints)

 

 

 

[shadow_ray]

Thank you for your time!

4 hours ago, wanderingfool2 said:

The only real note I would make though that might be a bit of a nit-picky, but naming the greater function as greater...instead of something like greater_int (to specify that it will be casting ints)

I totally agree.

 

4 hours ago, wanderingfool2 said:

Generalized functions can be useful to an extent, but the thing to remember as well it does come with a performance cost.  (Alas, that is always the trade-off isn't it).  So it would be important to realize where said function may be run (and how often).  Mainly because of the use of a functor, it means for each comparison a function has to be called...(so each element in the array would be 1 extra function and at least one extra register set...but likely more as it's sending the pointer as a parameter to the functor and the return value...less things that the compile time optimizer can do)

I think it depends on what one's trying to optimize. Buying a more powerful machine might be cheaper than paying for programmers to write optimized code which takes longer, requires good programmers or whatever. And from my little experience I think that choosing an inappropriate algorithm could cost exponentially more than a (small constant) * (input size) penalty in terms of performance. Writing generic, easy to read code and optimize when it's necessary seems like a good strategy to me, but as i said I don't have 'real world' experience. Would love to hear your opinion about this.

[shadow_ray]

I've done a little performance testing

Spoiler

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

int find_idx(const void* arr, 
    int arr_size, 
    int type_size, 
    bool (*cmp)(const void*, const void*)){

    int index = 0;
    for (int i = 1; i < arr_size; ++i){
        const void* a_ptr = (char*)arr + type_size*i; // &arr[i]
        const void* b_ptr = (char*)arr + type_size*index; // &arr[index]
        if( cmp(a_ptr, b_ptr))
            index = i;
    }
    return index; 
} 

bool greater_int(const void* a, const void* b){
    return *((int*)a) > *((int*)b);
}

int main() {
    srand(time(NULL));

    int arr_size = 100e6; 
    int* arr = (int*) malloc(arr_size * sizeof(int));

    for(int i = 0; i < arr_size; ++i){
        arr[i] = rand();
    }
	
    clock_t start, end;
    start = clock();

    int idx = find_idx(arr, arr_size, sizeof(int), greater_int);

    end = clock();

    printf("index: %d, value: %d, time: %d", idx, arr[idx], (end-start));

    free(arr);
    return 0;
}

 

Spoiler

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

int max_idx(int arr[], int size){
  
    int index = 0;
    for (int i = 1; i < size; ++i)
        if (arr[i] > arr[index])
            index = i;

    return index; 
} 


int main() {
    srand(time(NULL));

    int arr_size = 100e6; 
    int* arr = (int*) malloc(arr_size * sizeof(int));

    for(int i = 0; i < arr_size; ++i){
        arr[i] = rand();
    }
	
    clock_t start, end;
    start = clock();
   
    int idx = max_idx(arr, arr_size);
   
    end = clock();

    printf("index: %d, value: %d, time: %d", idx, arr[idx], end-start);
    free(arr);
    return 0;
}

 

 

Here are the results. Compiler: gcc 8.1 (MinGW-W64), array_size 100e6 (one hundred million) ints ~ 400MB

+----------+--------------+--------------+
| program  | optimization | runtime (ms) |
+----------+--------------+--------------+
| general  | -O1          | 650          |
|          +--------------+--------------+
|          | -O2          | 450          |
|          +--------------+--------------+
|          | -O3          | 95           |
+----------+--------------+--------------+
| specific | -O1          | 415          |
|          +--------------+--------------+
|          | -O2          | 95           |
|          +--------------+--------------+
|          | -O3          | 95           |
+----------+--------------+--------------+

No performance difference between the two using -O3, but the assembly code is quite different: https://godbolt.org/z/vqTosr

 

There might be a difference with smaller arrays where the array fits into cache.

[wanderingfool2]

18 hours ago, shadow_ray said:

I think it depends on what one's trying to optimize. Buying a more powerful machine might be cheaper than paying for programmers to write optimized code which takes longer, requires good programmers or whatever.

Oh yea, I totally agree...most of the time it doesn't matter and the generalized solution (or rather the solution that was quicker to program for one off programs) are the best options.  It's always good though to keep a mental note for when you are running code that requires a lean function (like in a video game, in the physics calculation or something...where a few % of performance could be impactful)...but that's very specialty stuff anyways.

 

13 hours ago, shadow_ray said:

I've done a little performance testing

Haha, I was too lazy to do it (I was thinking about it yesterday, but didn't want to fire up my dev computer)...glad you did it.

 

13 hours ago, shadow_ray said:

No performance difference between the two using -O3, but the assembly code is quite different: https://godbolt.org/z/vqTosr

That's actually interesting...would have thought there would have been at least a bit more of a difference, even at the O3 level (although I guess with predictive branching and stuff maybe more modern CPU's don't have as much of an issue).  The O1 vs O3 was interesting to see (on basic form it called to memory so much more).  [Haven't really done optimization in a long time...makes me wonder though if the mov operation with multiplication eats up a lot more clock cycles...as the general solution using just mov with addition].

 

*Haven't run it, if I remember tomorrow I might try*

Spoiler

#include <stdbool.h>
#include <stdio.h>

//Under -O3, the assembly only has an additional cost of 1 added add, but if my theory is right it saves on 1 multiplication (per loop)
//So it benefits from just being able to call the memory location...thus should be more optimized.
int max_idx(int arr[], int size){
    int index = 0;
    const int* pArr = arr+1;
    int bestIndex = arr[0];
    for (int i = 1; i < size; ++i) {
        if (*pArr > bestIndex) {
            bestIndex = *pArr; //This actually adds only 1 extra line to the assembly code, but eliminates the constant multiplication
            index = i;
        }
        ++pArr;
    }

    return index; 
} 

int main() {
  
    int arr[] = {1, 2, 10, 3, 1, 0};
    int arr_size = sizeof(arr)/sizeof(int);
    
    int idx = max_idx(arr, arr_size);
    printf("MAX index: %d, value: %d\n", idx, arr[idx]);
    
    return 0;
}

 

 

[shadow_ray]

On 1/6/2021 at 8:28 AM, wanderingfool2 said:

That's actually interesting...would have thought there would have been at least a bit more of a difference, even at the O3 level (although I guess with predictive branching and stuff maybe more modern CPU's don't have as much of an issue).  The O1 vs O3 was interesting to see (on basic form it called to memory so much more).  [Haven't really done optimization in a long time...makes me wonder though if the mov operation with multiplication eats up a lot more clock cycles...as the general solution using just mov with addition].

 

The data I gathered is really noisy. OS is running in the background the scheduler is doing its thing, it's hard to tell

 

[wanderingfool2]

8 hours ago, shadow_ray said:

 

The data I gathered is really noisy. OS is running in the background the scheduler is doing its thing, it's hard to tell

 

Yea, tried it in Linux under a VM...best I could say is it was on par with the rest...I think that the processor must be doing some speculative magic, since the instruction on the "optimized" one does have less instructions than the "generalized" one (so in theory would be slower)...with that said I agree, too much noise when I tested it...but I think the general conclusion that I came to is that modern processors and also the compiler optimizations have come a long way.