Thread: Pointers and functions

Why is it that when I pass a regular charcter array:

Code:

char optionValue[20];

as the return value to a function that uses a character pointer as it's return type:

Code:

char *funcName(int a, int b)
{
 char optionValue[20];

 filler code;

 return optionValue;
}

My compiler, (Gcc under Linux) gives me the warning, "Function 'funcName' returns adress of local variable", then, in the output, when the return variable is printed, it's just a bunch of random characters?

Originally posted by erikj
the compiler is telling you what is wrong. Why not trust it?

you can't return the pointer since it gets out of scope.

Instead do this

Code:

char *funcName(int a, int b)
{
 char *optionValue=malloc(sizeof(char)*20);

 filler code;

 return optionValue
}

No freakin' way! Sorry -- malloc is NOT always the answer to problems of this type -- especialy this one.

Code:

char *funcName(int a, int b)
{
    static char optionValue[20];
    filler code;
    return optionValue
}

static makes the array stick around and will only have one instance of the array, whereas malloc will generate a new array each and every time the function is called and must be freed sometime after funcName returns and before it is called again.

as always allocated mem needs to be deallocated.

If you are using static, subsequent calls will overwrite the first call.

Imagine this

str1=funcName(somevalues);
str2=funcName(somevalues);

the seconds time str1 gets changed too since it points to the same memory as str2.

Why use a function if you only can call it once?

and before it is called again.

That is not true, sorry.

I tried using the static array, but I need to be able to change the variable in the rest of the function. Here's the whole program (it's not very big) if this clarifies anything:

http://roint.sdf1.org/about.c

It's just some code I wrote that extracts the values from a configuration file.

The overall best solution to where to store strings is to do it in the same way that most string functions do, and that is make the caller supply the storage of where the string is to be stored. You already use fgets() as a prime example of this technique, so use the same idea

Like so.

Code:

/*
 * About, a program for reading information
 * from a configuration file.
 * By Joel Glidden,
 */

#include<stdio.h>
#include<string.h>

char *cfgOption(char *optionKey, FILE *cfgFile, char *optionValue, size_t optionLen);

int main()
{
    FILE *cfgFile;
    char *myName;
    char optionValue[20];

    cfgFile = fopen("about.conf", "r");
    myName = cfgOption("UserName: ", cfgFile, optionValue, sizeof(optionValue) );

    printf("My name is %s.\n", myName);
    printf("My name is %s.\n", optionValue);    /* Same thing */
    fclose(cfgFile);

    return 0;
}

/*
 * You could make it more like fgets(), by say returning NULL if optionKey
 * isn't found in the file.
 */
char *cfgOption(char *optionKey, FILE *cfgFile, char *optionValue, size_t optionLen)
{
    char    buff[BUFSIZ];
    size_t  Keylen = strlen(optionKey);

    optionValue[0] = '\0';  /* just in case nothing is found */
    
    while( fgets(buff, sizeof buff, cfgFile) != NULL)
    {
        char    *nl = strchr( buff, '\n' ); /* find a newline */
        if ( nl != NULL ) *nl = '\0';       /* remove it if necessary */
        if ( strncmp( buff, optionKey, Keylen ) == 0 )
        {
            /* copy to user store, but don't overflow if it won't fit */
            strncpy( optionValue, &buff[Keylen], optionLen );
            optionValue[optionLen-1] = '\0';
            break;
        }
    }
    rewind(cfgFile);
    return optionValue; /* this is OK, it was a parameter after all */
}

Originally posted by erikj
as always allocated mem needs to be deallocated.

If you are using static, subsequent calls will overwrite the first call.

Imagine this

str1=funcName(somevalues);
str2=funcName(somevalues);

the seconds time str1 gets changed too since it points to the same memory as str2.

Why use a function if you only can call it once?

Fairly obvious. The functional spec we were given does not assume the previous string must stay. And even if it did, it is always a good idea when handed a char* to move it into a buffer under your control and out of the routine's control. A simple strcpy() is perfect.

Originally posted by erikj
"and before it is called again."
That is not true, sorry.

What's not true.... Oh, my full statement
"whereas malloc will generate a new array each and every time the function is called and must be freed sometime after funcName returns and before it is called again."
Again -- what's not true?
1) malloc will generate a new array each and every time the function is called... -- I beg to differ, this is absolutely true. And you will have to remember the pointer returned from each and every call to the function so you can free it later. The routine is high-maintenance.
2) ... and must be freed sometime after funcName returns and before it is called again. -- True, it is not a requirement that is be freed if
A) you don't mind memory leaks
B) you copy the pointer to another place so you don't lose it, so it can be freed later. Note strcpy() above accomplished the same thing. You had better copy something. If the buffer is small (as most are) strcpy() is safely sufficient.

[i]Originally posted by Roint /i]
I tried using the static array, but I need to be able to change the variable in the rest of the function.

You've been given the pointer -- you can change the contents.

Or, as I mentioned above, it's best you copy the buffer to your own buffer (consider the function as a utility someone else wrote) for further manipulation.

Thanks, everyone, you've all helped me a lot.

- Roint

I just have one more question; why exactly did you use this to obtain the option's value:

Code:

if ( strncmp( buff, optionKey, Keylen ) == 0 )
{
  strncpy( optionValue, &buff[Keylen], optionLen );
  optionValue[optionLen-1] = '\0';
  break;
}

Rather than this:

[code]
while((buff = fgetc(cfgFile)) != '\n')
{
sprintf(valueChar, "%c", cfgBuff2);
strcat(optionValue, valueChar);
}

> why exactly did you use this to obtain the option's value:
Because its
a) quicker
Using sprintf(valueChar, "%c", cfgBuff2); to create a string out of a single character is extraordinarily expensive.
valueChar[0] = cfgBuff2;
valueChar[1] = '\0';
Is much quicker, but you're still calling a function for every character.

b) works
Your optionValue array is uninitialised, which means the strcat would most likely start appending characters at some random memory location.

Study your code carefully - you only scan up to the next newline if you successfully match a key.

If you have in your config file something like
somekeyname: somevalue
Your first fgets() call reads 10 chars (the length of "UserName: "), so it would read "somekeynam".
This doesn't match, so you read the next 10 chars from the SAME line, ie "e: someval"
The third fgets() call would actually find a newline, so it would just return "ue\n".

I see. Thanks again

And just to round out your information (to Roint), typically the reason you don't normally return the address of a local variable is because local variables are generally allocated on the stack, which is temporary/changing.

As soon as you exit your function, the stackframe for the function is no longer valid and it's memory space can get walked on by the stack again-- hence you see "junk characters"...

Understanding how RAM is used is critical to writing stable code.

Happy New Year.