C code to strip a file's extension from a filename

[lotios611]

Code: Select all

char* stripFileExtension(char* fileNameToStrip)
{
	int sizeOfStrippedFileName = 0;
	int i;
	for (i = 0; i <= (int)strlen(fileNameToStrip); i++)
	{
		if (fileNameToStrip[i] != '.')
		{
			++sizeOfStrippedFileName;
		}
		else
		{
			break;
		}
	}
	char* strippedFileName = malloc(sizeOfStrippedFileName * sizeof(char));
	strncpy(strippedFileName, fileNameToStrip, sizeOfStrippedFileName);
	return strippedFileName;
}

I'm currently writing a Brainfuck compiler, and I needed to be able to separate the actual name of the file from the file extension and coded up this bit of code. It should be fairly easy to understand, but if you have any questions, ask away.

Edit: Changed the title so it makes more sense

[Ginto8]

EDIT: oops completely misread what it was supposed to do, sorry about that

[avansc]

not to steal your thunder, but I realized some optimization could be made, and came up with this:

Code: Select all

const char* getFileExtension(const char* filename) {
    const char* tmp=strchr(filename,'.'),*ret = NULL;
    while(tmp) {
        ret = tmp;
        tmp = strchr(tmp+1,'.');
    }
    return ret?ret+1:NULL;
}

then if you wanted a separately allocated string, you could do this:

Code: Select all

char* extension = strdup(getFileExtension("blarg.txt"));

and it would allocate a string on the heap (same contents as what's returned by getFileExtension), which you would then later return with free().

does not do the same thing no?

[Ginto8]

EDIT: oops

[avansc]

Well just to chime in.

you can go all silly and write it in all different ways.

something like

Code: Select all

char *stripExt(const char *file)
{
	int len = strchr(file,'.') - file;
	char *ret = (char*)calloc(sizeof(char), len+1);
	memcpy(ret, file, len);
	return ret;
}

1000000 calls takes 88.194000 ms as apposed to lots' that runs in like 101ms. that might as well be the same speed. since this function is hardly going to be called a million times.

If anything his is a bit better in terms of readability. ...anywho.

[M_D_K]

Well just to chime in.

you can go all silly and write it in all different ways.

something like

Code: Select all

char *stripExt(const char *file)
{
	int len = strchr(file,'.') - file;
	char *ret = (char*)calloc(sizeof(char), len+1);
	memcpy(ret, file, len);
	return ret;
}

1000000 calls takes 88.194000 ms as apposed to lots' that runs in like 101ms. that might as well be the same speed. since this function is hardly going to be called a million times.

If anything his is a bit better in terms of readability. ...anywho.

That could be a problem if there's more than one period in a filename, strrchr would be better.

[bnpph]

Don't put calloc in functions like that.
Have it pass a pointer and store the created string into it.


Also, the fastest method would be to just return the length of filename without extension, and then use that instead of returning a new string. You could replace it with null char if you want to.

[avansc]

Don't put calloc in functions like that.
Have it pass a pointer and store the created string into it.


Also, the fastest method would be to just return the length of filename without extension, and then use that instead of returning a new string. You could replace it with null char if you want to.

errr what now? You might have to explain what you mean by, "Have it pass a pointer and store the created string into it."

Also, what is special about that function that makes it bad to use calloc in it?

I used calloc so that I would not have to manually null terminate the string I just made. since calloc initializes the memory you allocated to 0.

also im not entirely sure what happens if you just insert a null terminator in terms of when that memory gets free. anywho this might be a moot point since im not sure how it deals with that. *edit* seems that this wont be an issue, since when the mem is allocated a "header" is attached.

[bnpph]

Code: Select all

char *stripExt(char* destination, const char *file)
{
   int len = strchr(file,'.') - file;
   strncpy(destination, file, len);
   return destination;
}

(might have errors - didn't test)

This is much better way. This function will work with any type of allocated memory, and is much less likely to have memory leaks. Try to avoid allocating (and not freeing them before function returns) inside functions that are not object oriented.

[avansc]

Code: Select all

char *stripExt(char* destination, const char *file)
{
   int len = strchr(file,'.') - file;
   strncpy(destination, file, len);
   return destination;
}

(might have errors - didn't test)

This is much better way. This function will work with any type of allocated memory, and is much less likely to have memory leaks. Try to avoid allocating (and not freeing them before function returns) inside functions that are not object oriented.

I see what you are saying, however I dont accept the premise, "and is much less likely to have memory leaks". In both implementations you still need to free the memory explicitly somewhere. I will concede that you are more likely to free the memory if you can see the allocation in the same code block, but its hardly a good argument, since something like strdup does what you say you should not.

strdup's function name is more indicative of what it does tho, but in the end the leak argument is poor, since it all boils down to the programmer and his or hers responsibility.

and besides, with that implementation, how do you know how big "destination" should be?

[bnpph]

stdup is not ISO C and I don't agree with it.
All of string.h's functions are designed where they pass a destination pointer. They also require your destination is large enough to store the result.

If you want to do what you're doing, it would make more sense to create a new type (FILENAME) as typedef of char*, then rename your function "createFilenameFromFile" or something, and then add a "deleteFilename" function. This is how many libraries do it.

Or, you can use C++ and wrap it in a class. That would be even better.

[avansc]

stdup is not ISO C and I don't agree with it.
All of string.h's functions are designed where they pass a destination pointer. They also require your destination is large enough to store the result.

If you want to do what you're doing, it would make more sense to create a new type (FILENAME) as typedef of char*, then rename your function "createFilenameFromFile" or something, and then add a "deleteFilename" function. This is how many libraries do it.

Or, you can use C++ and wrap it in a class. That would be even better.

So hang on, you want me to typedef a new type FILENAME, then write a function that does what?

Code: Select all

void deleteFileName(FILENAME file)
{
    free(file);
    file = NULL;
}

Id prefer not to, I have seen this in some source, but I can do that in the code thx. If this was something that I had to type a lot I would consider it. I get what you are aiming at, I just think that its hardly a good reason.

Here is some Quake3 source, now im not trying to advocate that its the best code in the world, or that their practices are law. Just wanted to point out that people do what you seem to think is a bad practice.

Code: Select all

char *copystring(const char *s)
{
	char	*b;
	b = malloc(strlen(s)+1);
	strcpy (b, s);
	return b;
}

Code: Select all

/*
================
AllocBspFace
================
*/
bspface_t	*AllocBspFace( void ) {
	bspface_t	*f;

	f = malloc(sizeof(*f));
	memset( f, 0, sizeof(*f) );

	return f;
}

Code: Select all

winding_t	*WindingFromDrawSurf( mapDrawSurface_t *ds ) {
	winding_t	*w;
	int			i;

	w = AllocWinding( ds->numVerts );
	w->numpoints = ds->numVerts;
	for ( i = 0 ; i < ds->numVerts ; i++ ) {
		VectorCopy( ds->verts[i].xyz, w->p[i] );
	}
	return w;
}

Anyways, this is hardly important enough to worry about it, to each his own.

[bnpph]

You will have to type free() a lot - everytime you use your function.
It would be a nightmare to have to change all those free()s if you changed your malloc implementation!

Code: Select all

/*
================
AllocBspFace
================
*/
bspface_t   *AllocBspFace( void ) {
   bspface_t   *f;

   f = malloc(sizeof(*f));
   memset( f, 0, sizeof(*f) );

   return f;
}

This is exactly what I'm talking about - the function specifically states in its name that it is allocating, it returns a typedef of its unique type, and most likely has a FreeBspFace() too.

This is safe, its name explains what it does, and the implementation can easily be changed without breaking anything. You also only have to include the "BspFace" header - its not mandatory to include stdlib wherever you use it.

Anyway, it's all personal preference, so do whatever you like.

[avansc]

Anyway, it's all personal preference, so do whatever you like.

Anyways, this is hardly important enough to worry about it, to each his own.

- paraphrased