Problem with C function of type char pointer, can someone explain?

Question

Can someone help me find the errors in this C function?

    char* f(int i) {
            int i; 
            char buffer[20];
            switch ( i ) {
                    1: strcpy( buffer, "string1");
                    2: strcpy( buffer, "string2");
                    3: strcpy( buffer, "string3");
                    default: 
                    strcpy(buffer, "defaultstring");
            }
            return buffer;
    }

I think it has to do with type conversion. My compiler gives a warning that 'the declaration of int i shadows a parameter'.

Answer 1

You have a parameter i and you are declaring a local variable i.

Be declaring a local variable with the same name as a parameter you can no longer access both. you should change the local variable name, or the parameter name.

Answer 2

Well, for one thing, you're missing the 'case' declaration in your switch statement as well as the breaks at the end of the statement. Try changing your code to this:

    char* f(int i) {
            int i; 
            char buffer[20];
            switch ( i ) {
                    case 1: strcpy( buffer, "string1"); break;
                    case 2: strcpy( buffer, "string2"); break;
                    case 3: strcpy( buffer, "string3"); break;
                    default: 
                    strcpy(buffer, "defaultstring"); break;
            }
            return buffer;
    }

Also, you're overriding your parameter i with a local variable named i. So remove that:

    char* f(int i) {
            char buffer[20];
            switch ( i ) {
                    case 1: 
                         strcpy( buffer, "string1"); 
                         break;
                    case 2: 
                         strcpy( buffer, "string2"); 
                         break;
                    case 3: 
                         strcpy( buffer, "string3"); 
                         break;
                    default: 
                         strcpy(buffer, "defaultstring"); 
                         break;
            }
            return buffer;
    }

Finally, you are returning a char pointer that points to a local statically declared character array named buffer. buffer falls out of scope as soon as the pointer to its first element is returned (what your return statement does). What that means is you end up with a pointer to a variable that no longer exists. To fix this, you can pass a pointer to buffer into the function, like this:

    void f(char *buffer, int i) {
            switch ( i ) {
                    case 1: 
                         strcpy( buffer, "string1"); 
                         break;
                    case 2: 
                         strcpy( buffer, "string2"); 
                         break;
                    case 3: 
                         strcpy( buffer, "string3"); 
                         break;
                    default: 
                         strcpy(buffer, "defaultstring"); 
                         break;
            }
    }

That last version should work, provided you make sure that buffer is long enough to accept the string. If you really want to make it safe, take in buffer's length as well and check that against the length of the string you are copying.

Answer 3

You declare a local variable i, which shadows the i parameter to the function.

Answer 4

A couple fixes applied here

    const char* f(int i) {
            switch ( i ) {
                    case 1: return "string1";
                    case 2: return "string2";
                    case 3: return "string3";
            }
            return "defaultstring";
    }

Answer 5

It does shadow a parameter. i is the function argument, then is declared inside function. Also, you can't declare an array on stack (char buffer[20]) then return it. It (buffer) goes out of scope as soon as the function exits.

Answer 6

The problem is exactly what it's telling you. Right at the beginning you have:

char* f(int i) {
        int i; 

The second line defines a variable named i -- which "shadows" (hides) the i that was passed as a parameter. When you do your switch on the value of i, you're doing it on the (uninitialized) value of the local variable, instead of the parameter you received.

You've also defined buffer local to f, so returning it is a problem -- it no longer exists by the time it's been returned. Given that what you're putting into it is any one of a number of string literals, you'd be better off returning the string literals directly:

char const *f(int i) { 
    switch (i) { 
        case 1: return "string 1";
        case 2: return "string 2";
        case 3: return "string 3";
        default: return "default string";
    }
}

Alternatively, you could use an array of strings:

char const *f(int i) { 
    char *strings[] = {"", "string 1", "string 2", "string 3"};
    if (i>0 && i<3) 
        return strings[i];
    return "default string";
}

Answer 7

You are returning a local variable - that can only end in tears.

Answer 8

  char* f(int i) {
            int i; 

Error 1: the local 'i' parameter shadows the 'i' argument to the function

        char buffer[20];
        switch ( i ) {

Error 2: You use the local 'i' variable, which is not initialized.

                1: strcpy( buffer, "string1");
                2: strcpy( buffer, "string2");
                3: strcpy( buffer, "string3");
                default: 
                strcpy(buffer, "defaultstring");
        }
        return buffer;

Error 3: You return a pointer to an element in a local array, that pointer is not valid when the function returns. 'buffer' has gone out of scope.

Answer 9

There are 3 big problems:

1. int i is declared both as a function parameter and a local automatic variable. Remove the int i; statement on line 2.
2. The case keyword must proceed each value in a switch statement (e.g. case 1: instead of 1:).
3. You are returning a pointer to an automatic variable. The char buffer[20]; variable is placed on the stack and is no longer valid after the function returns. Instead you should allocate memory on the heap and return a pointer to that.

Corrected function using strdup instead of strcpy:

char * f(int i) {
    switch (i) {
    case 1:
        return strdup("string1");
    case 2:
        return strdup("string2");  
    case 3:
        return strdup("string3");
    default:
        return strdup("defaultstring");
    }
}

Answer 10

There are a few problems here

1. You're redeclaring i on line 2. i is already the parameter to the function. You don't need to redeclare it.

2. Your syntax for the switch statement is incorrect. The correct syntax is something like this: case 1: /* do stuff */ break; case 2: /* do other stuff */ break;

3. You're attempting to return a pointer to a stack-allocated buffer. This will cause subtle problems, as there are no guarantees about that memory not being clobbered after the function returns. What is the purpose of the strcpys? If you just want to return those strings, just return those strings!

Here's what I'd do:

char* f(int i) {
        char buffer[20];
        switch ( i ) {
                case 1: return "string1";
                case 2: return "string2";
                case 3: return "string3";
                default: return "defaultstring";
        }
        return buffer;
}

Answer 11

1. You dont need a local int i if you have a int i given to you.

2. Instade of

   1: blabla;

you will need

case 1: blabla; break;

(for 1: 2: and 3: but not for the default)

3.I'm not sure about this (didn't do C for a long time) but I think the buffer will be empty after the end of the function. So you will have to pass down a char array (and the size) then change it. In that case you dont need do return anything.