returning char *

Hi. Can anyone tell me what the problem with this function is? It basically takes a file path, searches for the file name in it. It *should* return a pointer to the first character of the string (the file name). Here it is:

Code:

/*	returns pointer to file name from file path. */
char * fileName(const char * filePath)
{
	int i, j; char fileName[FILENAME_MAX+1];
	for (i = strlen(filePath), j=0 ; i > 0; i--, j++) 
	{
		if (filePath[i] == '\\') break;
		fileName[j] = filePath[i];
	}

	return fileName;
}

it gives me the following warning:
c:\abdallah\file encrypter (everything)\file encrypter vs\file encrypter\file encrypter\fileextension.c(74) : warning C4172: returning address of local variable or temporary.
When I try to output it using printf like this:

Code:

printf("%s", fileName(filename));

it doesn't output anything.

can anyone help me out?

You are, after all returning a local variable.

The problem with this is that the local variable is stored on the stack, above the return address back to the calling function. As soon as you return to the calling function, this memory is "freed".

You have (at least) four options. I've listed them as in the order I think are "best" to "worst":
1. Pass in a second parameter to use as an output buffer.
2. Copy the string in the calling function, and modify the incoming string.
3. Allocate memory using malloc, then return the pointer to this memory. Don't forget to free it after you've finished [this is why it's not a "top" suggestion].
4. Use a static variable e.g. static char fileName[...] - not very good for re-entrant [e.g. multithreaded or recursive use].

I'm not at all convinced your "clever" loop works, but perhaps it does - it's definitely not on the upper range of "easy to read" code.


I would also suggest that you don't have variables and functions with EXACTLY the same name - it can be VERY confusing in the long run.

--
Mats

Originally Posted by Abda92

Code:

/*	returns pointer to file name from file path. */
char * fileName(const char * filePath)
{
	int i, j; char fileName[FILENAME_MAX+1];
	for (i = strlen(filePath), j=0 ; i > 0; i--, j++) 
	{
		if (filePath[i] == '\\') break;
		fileName[j] = filePath[i];
	}

	return fileName;
}

fileName is destructed when the function ends, so you're returning a pointer to an invalid memory location on the stack.
Allocate memory on the heap using malloc, modify that memory and then return that pointer. Or take a buffer as argument to work with. Either works fine (I recommend the second).

Are you sure you want the file name to be reversed?

Todd

Yes. about the function body, I had made a few fixes but I couldn't post them. Here's the new code using the 1st solution from matsp:

Code:

int extractFileName(const char filePath[], char fileName[], size_t fileNameLength)
{
	int i, j;
	for (i = strlen(filePath); filePath[i] != '\\'; i--) 
		if (i == 0) return 0;
	i++;
	for (j=0 ; filePath[i] != '\0' && j < fileNameLength; j++, i++) 
		fileName[j] = filePath[i];
	fileName[j] = '\0';
	return 1;
}

But just to make things clear, you can not return a string from a function in any way?
Thanks

You can, if you allocate it on the heap, but it's usually considered better for the callee to pass in a buffer instead.
Also, since this is C, pass along the length of the buffer too, so you don't get buffer overruns.

Originally Posted by Abda92

But just to make things clear, you can not return a string from a function in any way?
Thanks

Not a local string (i.e. an array of chars). You can do either of these:

Code:

char* func()
{
    char* str = malloc( 80 * sizeof( char ) + 1 );
    strncpy( str, "Hello World", 80 );
    return str;  /* Now the calling function must free() the string. */
}

or

Code:

void func( char* str, unsigned int size )
{
    strncpy( str, "Hello World", (size - 1) );
}

The second method is usually better since the person calling the function knows for sure that they are responsible for memory allocation, plus then they also know the amount of memory allocated.

OK I corrected the code in my last post just in case someone wants it.

The whole point of passing along the size of the buffer is to make sure that you don't do a buffer overrun. Remember, that function has no idea how large the buffer is.
So go back and revise your code and make a check. When accessing fileName, check if you aren't accessing an element outside the length of the buffer.
In other words, if j == fileNameLength, then you have a problem because you're writing to the last element in the buffer and you need enough room to save a '\0' too.
If the buffer isn't enough for your function, ABORT.
You can also check in the beginning of the function if there's enough space.

Also, are you sure that you want to return zero when you don't find a backslash in the filename? Is "text.txt" not a valid input to your function?

--
Mats

to Elysia:
yes I did check for that in (j < fileNameLength). doesn't that make sure that we're not overrunning the buffer?

to matsp:
I did that on purpose. in the calling function I checked the return value to know what to output. e.g.

Code:

/* if rfilePath is actually the file name (as in typing the file name) extractFileName returns 0 */
if (extractFileName(wfilePath, fileName, FILENAME_MAX))
	printf("\n%s opened successfully\n", fileName);
else printf("\n%s opened successfully\n", wfilePath);

here, I input the file using wfilePath. But I don't want to show the user the whole path when I confirm it has succeeded in opening the file. If it is a path, I only want to show the file name. If it is a file name, then I want to show it.

Thanks for the help

Originally Posted by Abda92

to Elysia:
yes I did check for that in (j < fileNameLength). doesn't that make sure that we're not overrunning the buffer?

Actually, I believe there's a subtle bug in there.
Let's say that fileNameLength = 12.
That means the last valid index = 11 (since index start at 0).
So your array loops until j = 11, then the condition j < fileNameLength is met and the loop aborts, BUT j is incremented to 12 first (so j < fileNameLength is true).
So, then you do fileName[12] = '\0', that is one char beyond the array length.

As to returning values and functionality: I would much prefer to see a function that does a copy every time, and no checking of the return value at all [or use the return value to indicate success/fail if applicable].

The reason for this is that the code calling the function becomes much simpler.

--
Mats

You are working way too hard.

Code:

const char *fileName(const char *filePath)
{
    const char *slash = strrchr(filePath, '\\');
    return (slash && slash[1]) ? slash + 1 : NULL;
}

lol brewbuck.