C array of values, pointer segmentation fault

Question

In my main() function I create an array, find the address of the highest value and increase the value at the address:

int main()
{
    int i[5] = {2,5,6,5,3};
    int *pi = getAdres(i);
    (*pi)++;

    printf("%d", i[2]);
    return 0;
}

The getAdres() function looks like:

int getAdres(int *i)
{
    int *pi;
    int higest = 0;
    for(int j = 0; j < 5; j++)
    {
        if(i[j] > higest)
        {
            pi = &i[j];
            higest = i[j];
        }
    }
    return pi;
}

Without making the get address part into a function it works but in the current format (*pi)++; is giving me a segmentation fault. What is going wrong?

What is `if(i[j] > higest)` is never true? what are you returning then? — Sourav Ghosh, Dec 13 '17 at 12:40
Your function returns an `int` whereas it should actually return `int*`. hat probably happens is that `int*` gets truncated into `int` and thus causing segfault. — P.P, Dec 13 '17 at 12:41
@usr that's the thing!! A(ny) decent compiler should warn about the mismatch here, right? — Sourav Ghosh, Dec 13 '17 at 12:49

unwind · Answer 1 · 2017-12-13T13:08:10.437

You have the wrong return type, make it:

int * getAddressToMax(int *i);

Also make sure it does in fact return a valid pointer. If the entire array is 0, it will return an uninitialized pointer which is a really bad idea. Make it:

int *pi = i;

to make it return a pointer to the first element, in that case.

Also you commonly see:

int highest = i[0];

and then starting the loop from 1, for cases like this. Passing the length of the array is also very common in real code, hardcoding that kind of knowledge inside what could be a generic function makes things harder to understand (and kills code re-use, of course).

Vlad from Moscow · Accepted Answer · 2017-12-13T13:05:54.330

2

The function getAdres has the return type int instead of int *.

int getAdres(int *i)
^^^

But in any case its implementation is bad. It uses magic number 5 and moreover an arbitrary array passed to the function can have for example only negative values.

The function definition for example can look like

int * getMaxElement( const int *a, size_t n )
{
    size_t highest = 0;

    for ( size_t i = 1; i < n; i++ )
    {
        if ( a[highest] < a[i] )
        {
            highest = i;
        }
    }

    return ( int * )a + highest;
}

And the function can be called like

int *pi = getMaxElement( i, sizeof( i ) / sizeof( *i ) );
++*pi;

printf( "%d\n", *pi );

edited Dec 13 '17 at 13:05

answered Dec 13 '17 at 12:51

Vlad from Moscow

301,070
26
186
335

Fixed a small bug. `0` -> `a[0]` – klutt Dec 13 '17 at 13:03
@klutt You did not fixed a bug. You inserted a bug. – Vlad from Moscow Dec 13 '17 at 13:06
I realize that now. Misread your code. Sorry. – klutt Dec 13 '17 at 13:07
1

Dropping `const` like that is quite strange in my opinion. Better to return a `const` pointer, and have the caller cast it away (the caller knows that it's going to write, and also owns the array). – unwind Dec 13 '17 at 13:12
Or just remove const... because here that don't make any sense. cast that break const is... really a bad practice to teach. – Stargateur Dec 13 '17 at 13:17
@unwind Now you will know how such functions should be written in C.:) Also consider for example the standard C function strchr. – Vlad from Moscow Dec 13 '17 at 13:22
@Stargateur It is a good function implementation that satisfies the C Standard convention about such functions.:) – Vlad from Moscow Dec 13 '17 at 13:23
@VladfromMoscow I think that's very strange reasoning, and that `strchr()` looks like that for two reasons: history, and string literals. See [this answer](https://stackoverflow.com/questions/34842263/how-come-c-standard-library-function-strchr-returns-pointer-to-non-const-when). I'm certainly not willing to consider it a good design in general. – unwind Dec 13 '17 at 13:25
@unwind There is no function overloading in C. So it is a general approach to define functions such a way for constant and non-constant arrays. There is no need casting for a constant pointer and a non-constant pointer to accept the result. Otherwise your code will be overloaded with numerous castings that makes it difficult to read. So the implementation of the function I showed is the best implementation. – Vlad from Moscow Dec 13 '17 at 13:30
@VladfromMoscow I never said it wasn't a "good function implementation" but clearly a broken design function. There no way this should be considerate as a good practice, standard C library is obviously not perfect and this one is a great example of bad practice (and stupidity). You want two behaviors ? Do two functions. – Stargateur Dec 13 '17 at 13:32
@Stargateur Well you can make a proposal to the C Standard Committee to double almost all C string function declarations.:) By the way the function themselves have one and only one behavior. It is the client of the functions that have a choice.:) – Vlad from Moscow Dec 13 '17 at 13:34
@Stargateur And moreover it is a bad practice and as you said stupidity to write the same algorithm twice in two different places and having different names for the same algorithm.:) – Vlad from Moscow Dec 13 '17 at 13:39
@VladfromMoscow Let the caller of this function handle the cast would be mush more clean practice, but still prefer two function. This algorithm could be write in a macro that take a type `int *` or `int const *` and you will not write twice the same algorithm. I think committee are very aware of this problem but there don't want breaking change. – Stargateur Dec 13 '17 at 13:51

C array of values, pointer segmentation fault

2 Answers2