dynamic strings c

[Ruke]

hi, i recently began to program in c (i'm very noob) and i have problems appending chars to dynamic allocated strings.

i created this function to append strings:

code

char *appstr(char *s, const char *d)
{
	size_t slen = 0;
	size_t dlen = 0;
	char *ns;

	if (s)
		slen = strlen(s);

	if (d)
		dlen = strlen(d);

	ns = malloc(sizeof(char) * (slen+dlen+1));
	strcpy(ns, "");

	if (s)
		strcat(ns, s);

	if (d)
		strcat(ns, d);

	free(s);

	return ns;
}

and it works pretty well, except in this piece of code:

code

do {
		ch = fgetc(dependency_file);

		if (ch == '\n' || ch == EOF) {
			dependency = Dependency_create_from_url(dependency_url);
			Dependency_set_version(dependency, dependency_version);

			printf("%s,%s\n", dependency_url, dependency_version);

			if (DependencyContainer_add(dc, dependency) == DEPENDENCY_CONTAINER_VERSION_ERROR) {
				if (display_errors == 1) {
					printf(" [ERROR] Dependency %s is required in different versions\n", dependency->name);
				}
			}

			free(dependency_url);
			free(dependency_version);

			dependency_url = NULL;
			dependency_version = NULL;

			in_version = 0;
		} else if (ch == ',') {
			in_version = 1;
		} else {
			if (in_version == 0) {
				dependency_url = appstr(dependency_url, &ch);
			} else {
				dependency_version = appstr(dependency_version, &ch);
			}
		}
	} while(ch != EOF);

which reads a file with a format url,version\n...

i'm testing it with this test-file:

[email protected]:Ruk33/WurstBuff.git,HEAD
[email protected]:Ruk33/WurstBuff.git,1234

and prints:

[email protected]:Ruk33/WurstBuff.git,HEAD
<corrupted/incorrect string>,<corrupted/incorrect string>

where <corrupted/incorrect string> is a bunch of weird characters.

as you can see, the first print is correct, but the second is wrong and i can't spot the problem out.

can anybody help me with this?

thanks, greetings.

 

[Ruke]

fixed:

code

	do {
		ch = fgetc(dependency_file);

		if (ch == '\n' || ch == EOF) {
			dependency_url = setstr(line);
			dependency_version = strrchr(line, ',');

			if (dependency_version) {
				// Remove version from line so we get only the url
				dependency_url[strlen(dependency_url) - strlen(dependency_version)] = '\0';

				// Remove ","
				dependency_version++;
			}

			dependency = Dependency_create_from_url(dependency_url);
			Dependency_set_version(dependency, dependency_version);

			if (DependencyContainer_add(dc, dependency) == DEPENDENCY_CONTAINER_VERSION_ERROR) {
				if (display_errors) {
					printf(" [ERROR] Dependency %s is required in different versions\n", dependency->name);
				}
			}
			
			free(line);
			free(dependency_url);
			free(dependency_version);

			line = NULL;
			dependency_url = NULL;
			dependency_version = NULL;
		} else {
			line = appstr(line, &ch);
		}
	} while(ch != EOF);

 

[Dr Super Good]

You should be using "realloc" instead of malloc to resize. This is because it might be able to resize without requiring a move which could save memory copies. Especially useful if you are appending a lot of small strings as memory allocation might be done in blocks of some granularity (a little buffer space between needing to move the entire string).

You should also be testing if the string pointer are not the NULL macro instead of if they are logically true. Although for most platforms both approaches work (since NULL is defined as 0 and logically true is defined as anything not 0) however this need not always be the case. It might seem to make the code more complicated (now using a comparison) however in reality the compiler (if it is good like gcc) will optimize that away.

Generally you want to avoid function calls which malloc memory and return it expecting the caller to free it when it is done as that couples implementation details to the caller (maybe the caller uses a different dynamic memory system?). Instead you should have functions which deal with the deallocation of memory returned by other complex functions, even if it deals with the deallocation of memory returned by many different functions. If it turns out the implementation is simply to free the argument the compiler should have no problem with in-lining it so in the end the performance is the same.

 

[Ruke]

thanks doctor .

broken code

at the first i was using realloc, but it was throwing me some weird errors. i give it another try and i came up with this:

#include <stdlib.h>
#include <string.h>

#ifndef DSTR_H_INCLUDED
#define DSTR_H_INCLUDED

char *dstrcpy(char *source, const char *to_copy)
{
	if (source != NULL) {
		free(source);
	}

	if (to_copy != NULL) {
		source = malloc(sizeof(char) * (strlen(to_copy)+1));
		source = strcpy(source, to_copy);
	} else {
		source = malloc(sizeof(char));
		source = strcpy(source, "");
	}

	return source;
}

char *dstrcat(char *source, const char *append)
{
	if (source == NULL) {
		source = dstrcpy(source, "");
	}

	if (append != NULL) {
		source = realloc(source, sizeof(char) * (strlen(source)+strlen(append)+1));
		source = strcat(source, append);
	}

	return source;
}

char *dstrcatc(char *source, const char append)
{
	if (source == NULL) {
		source = dstrcpy(source, "");
	}

	if (append != '\0') {
		source = realloc(source, sizeof(char) * (strlen(source)+2));

		source[strlen(source)] = append;
		source[strlen(source)+1] = '\0';
	}

	return source;
}

#endif

tests:

#include "assert.h"
#include "../dstr.h"

void main()
{
	char *s = NULL;
	char *a = NULL;
	char ch = ',';

	s = dstrcpy(s, "something");
	ec("something", s);

	s = dstrcpy(s, "lorem");
	ec("lorem", s);

	a = dstrcat(a, NULL);
	ec("", a);

	a = dstrcat(a, "im");
	ec("im", a);

	a = dstrcat(a, " awesome");
	ec("im awesome", a);

	a = dstrcatc(a, ch);
	a = dstrcatc(a, ch);
	a = dstrcatc(a, '\n');
	ec("im awesome,,\n", a); // ---> FAILS a == "im awesome,,\n<weird character>

	free(s);
	free(a);
}

note: ec(e,g) -> equal_char(expected, given)

greetings and thanks again .

edit: nvm about -2, if i alloc another char it fails.

working code

edit: now it works perfectly with sprinf:

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

#ifndef DSTR_H_INCLUDED
#define DSTR_H_INCLUDED

char *dstrcpy(char *source, const char *to_copy)
{
	if (source != NULL) {
		free(source);
	}

	if (to_copy != NULL) {
		source = malloc(sizeof(char) * (strlen(to_copy)+1));
		source = strcpy(source, to_copy);
	} else {
		source = calloc(0, sizeof(char));
	}

	if (source[strlen(source)+1] != '\0') {
		source[strlen(source)+1] = '\0';
	}

	return source;
}

char *dstrcat(char *source, const char *append)
{
	if (source == NULL) {
		source = dstrcpy(source, NULL);
	}

	if (append != NULL) {
		source = realloc(source, sizeof(char) * (strlen(source)+strlen(append)+1));
		source = strcat(source, append);
	}

	return source;
}

char *dstrcatc(char *source, const char append)
{
	if (source == NULL) {
		source = dstrcpy(source, NULL);
	}

	if (append != '\0') {
		source = realloc(source, sizeof(char) * (strlen(source)+2));
		sprintf(source, "%s%c", source, append);
	}

	return source;
}

#endif

tests:

#include "assert.h"
#include "../dstr.h"

void main()
{
	char *copy_str = NULL;
	char *append_str = NULL;
	char ch = 'c';

	copy_str = dstrcpy(copy_str, "something");
	ec("something", copy_str);

	copy_str = dstrcpy(copy_str, "lorem");
	ec("lorem", copy_str);

	copy_str = dstrcpy(copy_str, NULL);
	ec("", copy_str);
	a(copy_str[strlen(copy_str)+1] == '\0', "copy_str doesnt have eof");

	append_str = dstrcat(append_str, NULL);
	ec("", append_str);
	a(append_str[strlen(append_str)+1] == '\0', "append_str doesnt have eof");

	append_str = dstrcat(append_str, "im");
	ec("im", append_str);

	append_str = dstrcat(append_str, " awesome");
	ec("im awesome", append_str);

	append_str = dstrcatc(append_str, ch);
	append_str = dstrcatc(append_str, ch);
	append_str = dstrcatc(append_str, '\t');
	ec("im awesomecc\t", append_str);
	a(append_str[strlen(append_str)+1] == '\0', "a doesnt have eof");

	free(copy_str);
	free(append_str);
}

 

[GhostWolf]

What on earth is the point of any of that code? why are you reimplementing core functions in terrible ways...

On a side note, sizeof(char) is defined as 1. Always.

 

[Ruke]

new version

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

#ifndef DSTR_H_INCLUDED
#define DSTR_H_INCLUDED

char *dstrcpy(char *source, const char *to_copy)
{
	size_t to_copy_len = 0;

	if (to_copy != NULL) {
		to_copy_len = strlen(to_copy);
	}

	if (source == NULL) {
		source = malloc(sizeof(char) * (to_copy_len+1));
	} else {
		source = realloc(source, sizeof(char) * (to_copy_len+1));
	}

	if (to_copy != NULL) {
		source = strcpy(source, to_copy);
	} else {
		source = strcpy(source, "");
	}

	return source;
}

char *dstrcat(char *source, const char *append)
{
	if (source == NULL) {
		source = dstrcpy(source, NULL);
	}

	if (append != NULL) {
		source = realloc(source, sizeof(char) * (strlen(source)+strlen(append)+1));
		source = strcat(source, append);
	}

	return source;
}

char *dstrcatc(char *source, const char append)
{
	size_t slen;

	if (source == NULL) {
		source = dstrcpy(source, NULL);
	}

	slen = strlen(source);

	if (append == '\0' && source[slen+1] == '\0') { // avoid multiple eof

	} else {
		source = realloc(source, sizeof(char) * (slen+2));
		sprintf(source, "%s%c", source, append);
	}

	return source;
}

#endif

What on earth is the point of any of that code? why are you reimplementing core functions in terrible ways...

dynamic char allocation.

char *s = NULL;

s = dstrcpy(s, "something with unkown length");
s = dstrcat(s, " keep setting");
s = dstrcatc(s, '\t');

if there is any core functions to do that i didn't noticed, could you link me to them pls?

thanks.

edit: is really that bad?, could you give me any advices or point me where you think its bad? thanks again.

 

[Dr Super Good]

On a side note, sizeof(char) is defined as 1. Always.

However for readability it might still be a good idea to use since it explicitly tells you that you are dealing with an array of char as opposed to some number of bytes.

When you see a malloc being passed an argument that is a multiple of "sizeof(char)" it is clear that you are dealing with an array of char. If you just see some quantity of bytes being allocated there is no readable attached purpose to the allocation so it could potentially lead to errors.

The code generated at the end should be no different as compilers mostly optimize out things like constant multiply by 1 so to use it or not is entirely a programming style choice.

 

[edo494]

sizeof is compile time constant in all cases, so it doesnt matter anyways