Concat undefined number of strings

[dj51_Shura]

Hello everyone!

I need to concat an undefined number of strings, so I created a function called concat. I need to use va_list structures, but I have a problem while using va_arg macro. This is the code:

Code: Select all

char * concat(const char * str1, const char * str2, ...) {
	/* Create argument list */
	/* args */ va_list args;
	/* argn */ int argn = 2;
	
	/* Start argument list */
	va_start(args, str2);
	
	/* Get the lenght of all the strings */
	int bufsize = strlen(str1) + strlen(str2);
	
	while(/*  -- what? --  */) {
		/* Update the buffer size */
		bufsize += strlen(va_arg(args, char *));
		
		/* Advance the argument counter */
		argn++;
	}
	
	/* Add two bytes more for each argument */
	bufsize += argn*2;
	
	/* Create the buffer */
	char buffer[bufsize];
	memset(buffer, 0, bufsize);
	
	/* Copy to it the two initial strings */
	strcpy(buffer, str1);
	strcat(buffer, str2);
	
	/* Restart the arguments list */
	va_start(args, str2);
	
	while(/*  -- what? --  */) {
		/* Concat the string to the buffer */
		strcat(buffer, va_arg(args, char *));
	}
	
	/* Close the arguments list */
	va_end(args);
	
	/* End the buffer */
	buffer[bufsize] = '\0';
	
	/* Return the concatenation */
	return buffer;
}

As you can see, I have a problem with the loops. The idea is to end the argument list by passing NULL as last parameter of concat, but, how can I make sure of that if I can't get a parameter without advancing the list???

Yes, I can use a temporal buffer; I can copy to it the argument obtained with va_arg and then, gets its longer after checking if is NULL, but... how long should I make that temporal buffer? A macro that indicates maximum lenght of strings? Nah, I want a clean, fast and low-memory-use function.

Can you help me, please?

Thanks everybody!

Advertising

[m0skit0]

Why don't you just pass an array of char pointers and its size?

Advertising

[codestation]

As you can see, I have a problem with the loops. The idea is to end the argument list by passing NULL as last parameter of concat, but, how can I make sure of that if I can't get a parameter without advancing the list???

You need to advance the list to get the va_arg parameter, so why not try something like this?

Code: Select all

.....
char *va_param;
....
while((va_param = va_arg(args, char *))) {
    bufsize += strlen(va_param);
}

So you can extract the parameter and use it to compare aganist null at the same time.

Yes, I can use a temporal buffer; I can copy to it the argument obtained with va_arg and then, gets its longer after checking if is NULL, but... how long should I make that temporal buffer? A macro that indicates maximum lenght of strings? Nah, I want a clean, fast and low-memory-use function.
Thanks everybody!

I don't get you with this one, why don't use bufsize + 1 (for the 0-byte terminator) as your buffer size?

note: never return a pointer to a local variable, that will lead to undefined behavior. Use malloc to get your buffer, then free it outside your function when you are done with it.

[dj51_Shura]

I have solved! Thanks both of you!

This is now the code. As Codestation explains to me, I have created a temporal buffer to store the concatenation in it, but, just a question, how to free it before return? xD

Code: Select all

char * concat(const char * str1, const char * str2, ...) {
	/* Create argument list */
	/* args */ va_list args;
	/* argn */ int argn = 2;
	
	/* Start argument list */
	va_start(args, str2);
	
	/* Create a temporal buffer, allocate enough memory for it
	 * and fill it with null bytes */
	char * tempbuffer;
	tempbuffer = (char *)malloc(sizeof(char *) + MAXLENSTR);
	memset(tempbuffer, 0, sizeof(char *) + MAXLENSTR);
	
	/* Get the lenght of all the strings */
	int bufsize = strlen(str1) + strlen(str2);
	
	while((tempbuffer = va_arg(args, char *))) {
		/* Update the buffer size */
		bufsize += strlen(tempbuffer);
		
		/* Advance the argument counter */
		argn++;
	}
	
	/* Add two bytes more for each argument */
	bufsize += argn*2 + 2;
	
	/* Create the buffer */
	char buffer[bufsize];
	memset(buffer, 0, bufsize);
	
	/* Copy to it the two initial strings */
	strcpy(buffer, str1);
	strcat(buffer, str2);
	
	/* Restart the arguments list */
	va_start(args, str2);
	
	while((tempbuffer = va_arg(args, char *))) {
		/* Concat the string to the buffer */
		strcat(buffer, tempbuffer);
	}
	
	/* Free temporal buffer */
	free(tempbuffer);
	
	/* Close the arguments list */
	va_end(args);
	
	/* Define a pointer to return, allocate enough space to it
	 * and fill it with null bytes */
	char * fbuffer;
	fbuffer = (char *)malloc(strlen(buffer) + 2);
	memset(fbuffer, 0, strlen(buffer) + 2);
	
	/* Loop to get final buffer */
	int X;
	for(X = 0; X <= strlen(buffer); X++) {
		/* Assign */
		*fbuffer = buffer[X];
		/* Advance */
		fbuffer++;
	}
	
	/* Rewind the pointer */
	fbuffer -= strlen(buffer) + 1;
	
	/* Return the pointer */
	return fbuffer;
}

Thanks, best regards.

[codestation]

I have solved! Thanks both of you!

This is now the code. As Codestation explains to me, I have created a temporal buffer to store the concatenation in it, but, just a question, how to free it before return? xD

Eh? why would you want to free it before return? isn't the whole point of strcat to return the result of the concatenation? Not gonna check your code this time you you can have some thought of it and rewrote it to your real needs.

@m0skit0, i was 100% sure that there wasn't any post above mine before and after that i posted, what sorcery is this?

[dj51_Shura]

Codestation wrote:
Eh? why would you want to free it before return? isn't the whole point of strcat to return the result of the concatenation? Not gonna check your code this time you you can have some thought of it and rewrote it to your real needs.

Yes, fbuffer IS the result of the concat, but I've allocated memory for fbuffer with malloc, so I should free it with free, isn't it?

Well, that is my... understanding... sorry if I miss.

And, about the postscript

[codestation]

Yes, fbuffer IS the result of the concat, but I've allocated memory for fbuffer with malloc, so I should free it with free, isn't it?

Well, that is my... understanding... sorry if I miss.

Yes, you are right, you are supposed to free any allocated memory with malloc but i thought that you where doing something like this:

Code: Select all

char *result = concat("string1", "string2", "string3", "stringN");
/* do something with the concatenated string */
free(result);

[dj51_Shura]

No, I'm talking about the concat code. You said to me that I can't return adresses of local variables; so, simply, I create a final-buffer (fbuffer) and malloc strlen(buffer) + 1 to it; then copy the buffer into the final buffer and return the last one.

Code: Select all

    /* Define a pointer to return, allocate enough space to it
    * and fill it with null bytes */
   char * fbuffer;
   fbuffer = (char *)malloc(strlen(buffer) + 2);
   memset(fbuffer, 0, strlen(buffer) + 2);
   
   /* Loop to get final buffer */
   int X;
   for(X = 0; X <= strlen(buffer); X++) {
      /* Assign */
      *fbuffer = buffer[X];
      /* Advance */
      fbuffer++;
   }
   
   /* Rewind the pointer */
   fbuffer -= strlen(buffer) + 1;
   
   /* Return the pointer */
   return fbuffer;

But, I have used malloc... etc...

[m0skit0]

In fact you should not do mallocs inside functions like this. You need to pass to the function the buffer where you want the result stored, and do the malloc() thing BEFORE calling the function. Doing mallocs inside one functions and frees on another part of the code is bad. Try to keep mallocs and frees into the same code entity.

Also passing an array of char* is way simpler than what you've done, but anyway...

@m0skit0, i was 100% sure that there wasn't any post above mine before and after that i posted, what sorcery is this?

Your 100% scares me mate...

[codestation]

In fact you should not do mallocs inside functions like this. You need to pass to the function the buffer where you want the result stored, and do the malloc() thing BEFORE calling the function. Doing mallocs inside one functions and frees on another part of the code is bad. Try to keep mallocs and frees into the same code entity.

It depends, one will need to calculate the lenght of all those strings beforehand everytime you need to call the function to allocate the required buffer or use one big enough to hold all of it. Also i din't see a problem with a function that does allocations by itself if it states clearly that the returned buffer must be freed (take strdup by example).

Also passing an array of char* is way simpler than what you've done, but anyway...

Just calling snprintf(buffer, BUFFER_SIZE, "%s%s%s%s%s", str1, str2, str3, str4, str5); for doing multiple concats is more simpler when you think of it