6
\$\begingroup\$

I am new at C programming, so I would appreciate it if you could highlight the problems in my code. My biggest concern is wrong memory allocation.

There are some code snippets, where I repeat my code. Is there another way to get rid of them?

Github repo link.

sstream.h

#pragma once

#include <stdlib.h>

typedef struct sstream sstream;

struct sstream {
    char* buffer;
    unsigned int length;
    unsigned int capacity;
};

sstream* sstream_create(char* str);

void sstream_free(sstream* str);

char* sstream_get(sstream* str);

unsigned int sstream_lenght(sstream* str);

unsigned int sstream_capacity(sstream* str);

char sstream_atIndex(sstream* str, int idx);

unsigned int sstream_findChar(sstream* str, char c);

void sstream_print(sstream* str);

sstream* sstream_copy(sstream* str);

void sstream_pushFrontChar(sstream* str, char c);

void sstream_pushFrontStr(sstream* str, sstream* new);

void sstream_pushBackChar(sstream* str, char c);

void sstream_pushBackStr(sstream* str, sstream* new);

void sstream_insertStr(sstream* str, unsigned int idx, sstream* new);

void sstream_popFront(sstream* str);

void sstream_popBack(sstream* str);

void sstream_erase(sstream* str, unsigned int idx, unsigned int len);

void sstream_replace(sstream* str, unsigned int idx, unsigned int len, sstream* new);

sstream* sstream_substring(sstream* str, unsigned int idx, unsigned int len);

sstream.c

#include "libs/sstream.h"

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

sstream* sstream_create(char* str) {
    sstream* result = malloc(sizeof(sstream));
    result->length = strlen(str);
    result->capacity = result->length * 2;
    result->buffer = malloc(result->capacity+1);
    memcpy(result->buffer,str,result->length);
    result->buffer[result->length] = '\0';
    return result;
}

void sstream_free(sstream* str) {
    free(str->buffer);
    str->buffer = NULL;
}

char* sstream_get(sstream* str) {
    return str->buffer;
}

unsigned int sstream_lenght(sstream* str) {
    return str->length;
}

unsigned int sstream_capacity(sstream* str) {
    return str->capacity;
}

char sstream_atIndex(sstream* str, int idx) {
    return str->buffer[idx];
}

unsigned int sstream_findChar(sstream* str, char c) {
    return strchr(str->buffer, c) - str->buffer;
}

void sstream_print(sstream* str) {
    printf("%s",sstream_get(str));
}

sstream* sstream_copy(sstream* str) {
    return sstream_create(str->buffer);
}

void sstream_pushFrontChar(sstream* str, char c) {
    if(str->capacity < str->length + 2) { //+1 c, +1 \0
        char* temp = realloc(sstream_get(str),str->capacity*2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
    }
    memmove(&str->buffer[1],&str->buffer[0],str->length+1);
    str->buffer[0] = c;
    str->length = str->length + 1;
    str->buffer[str->length] = '\0';
}

void sstream_pushFrontStr(sstream* str, sstream* new) {
    if(str->capacity < (str->length + new->length + 1)) { //+1 \0
        char* temp = realloc(sstream_get(str),str->capacity * 2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
        if(str->capacity < (str->length + new->length + 1)) {
            sstream_pushFrontStr(str,new);
            return;
        }
    }
    memmove(&str->buffer[new->length],&str->buffer[0],str->length);
    memmove(&str->buffer[0],&new->buffer[0],new->length);
    str->length = str->length + new->length;
    str->buffer[str->length] = '\0';
}

void sstream_pushBackChar(sstream* str, char c) {
    if(str->capacity < str->length + 2) { //+1 c, +1 \0
        char* temp = realloc(sstream_get(str),str->capacity*2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
    }
    str->length = str->length + 1;
    str->buffer[str->length - 1] = c;
    str->buffer[str->length] = '\0';
}

void sstream_pushBackStr(sstream* str, sstream* new) {
    if(str->capacity < (str->length + new->length + 1)) { //+1 \0
        char* temp = realloc(sstream_get(str),str->capacity * 2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
        if(str->capacity < (str->length + new->length + 1)) {
            sstream_pushBackStr(str,new);
            return;
        }
    }
    memmove(&str->buffer[str->length],new->buffer,new->length);
    str->length = str->length + new->length;
    str->buffer[str->length] = '\0';
}

void sstream_insertStr(sstream* str, unsigned int idx, sstream* new) {
    if(str->capacity < (str->length + new->length + 1)) { //+1 \0
        char* temp = realloc(sstream_get(str),str->capacity * 2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
        if(str->capacity < (str->length + new->length + 1)) {
            sstream_insertStr(str,idx,new);
            return;
        }
    }
    memmove(str->buffer,str->buffer,idx);
    memmove(&str->buffer[idx + new->length],&str->buffer[idx],str->length - idx);
    str->length = str->length + new->length;
    memmove(&str->buffer[idx],new->buffer,new->length);
    str->buffer[str->length] = '\0';
}

void sstream_popFront(sstream* str) {
    memmove(&str->buffer[0],&str->buffer[1],str->length-1);
    str->length = str->length - 1;
    str->buffer[str->length] = '\0';
}

void sstream_popBack(sstream* str) {
    memmove(&str->buffer[str->length],&str->buffer[str->length+1],str->length - 1);
    str->length = str->length - 1;
    str->buffer[str->length] = '\0';
}

void sstream_erase(sstream* str, unsigned int idx, unsigned int len) {
    memmove(&str->buffer[idx],&str->buffer[idx + len], str->length - idx);
    str->length = str->length - len;
    if(str->capacity > (str->length * 16)) { // if to big
        str->capacity = str->length * 2 + 1;
        str->buffer = realloc(str->buffer,str->capacity);
    }
    str->buffer[str->length] = '\0';
}

void sstream_replace(sstream* str, unsigned int idx, unsigned int len, sstream* new) {
    if(str->capacity < (str->length + new->length + 1)) { //+1 \0
        char* temp = realloc(sstream_get(str),str->capacity * 2);
        str->capacity = str->capacity * 2;
        str->buffer = temp;
        if(str->capacity < (str->length + new->length + 1)) {
            sstream_replace(str,idx,len,new);
            return;
        }
    }
    memmove(str->buffer,str->buffer,idx);
    memmove(&str->buffer[idx],new->buffer,idx + new->length);
    memmove(&str->buffer[idx + new->length], &str->buffer[idx + len], str->length + new->length - len);
    str->length = str->length + new->length - len;
    str->buffer[str->length] = '\0';
}

sstream* sstream_substring(sstream* str, unsigned int idx, unsigned int len) {
    sstream* result = malloc(sizeof(sstream));
    result->length = len;
    result->capacity = result->length * 2;
    result->buffer = malloc(result->capacity + 1);
    memcpy(result->buffer,&str->buffer[idx],result->length);
    result->buffer[result->length] = '\0';
    return result;
}
\$\endgroup\$
2
  • \$\begingroup\$ If your biggest concern is the memory correctness, it would be very helpful to show the test suite too (presumably you've run that under Valgrind or similar?) - then we'd be able to point out testing that you've missed. \$\endgroup\$ Commented May 16, 2021 at 11:57
  • \$\begingroup\$ Oh okay, I do not know about this program. \$\endgroup\$ Commented May 16, 2021 at 12:10

1 Answer 1

Reset to default
5
\$\begingroup\$

I don't see any need for the header to include <stdlib.h>. It should compile just fine without that, and only the implementation needs to include it.

unsigned int length;
unsigned int capacity;

However, perhaps we do want <stdint.h>, so that we can use size_t for these values (that's a more natural choice, and is what we get back from strlen(), for example).

It's good that you include sstream.h first in the implementation - that's a good test that the header is complete, and usable without prerequisites.

There's a severe lack of robustness when allocating memory. Any memory allocation can fail, and this code runs into undefined behaviour whenever malloc() or realloc() returns a null pointer. As an example:

sstream* sstream_create(char* str)
{
    sstream* result = malloc(sizeof *result);
    if (!result) {
        return result;
    }
    result->length = strlen(str);
    result->capacity = result->length * 2;
    result->buffer = malloc(result->capacity+1);
    if (!result->buffer) {
        free(result);
        return NULL;
    }
    memcpy(result->buffer, str, result->length);
    result->buffer[result->length] = '\0';
    return result;
}

Those two if statements are essential for correctness in low-memory situations.

Special care needs to be taken with realloc(). This is a typical anti-pattern:

    str->buffer = realloc(str->buffer,str->capacity);

If the realloc() fails, str->buffer will be null, and we no longer have a pointer to the memory it previously pointed to - that's a memory leak.

    char *new_buf = realloc(str->buffer,str->capacity);
    if (!new_buf) {
        /* handle the error here */
    } else {
        str->buffer = new_buf;
    }

We can make the code a little more readable by using += and ++ rather than repeating the variable. E.g.

str->length = str->length + 1;

str->length = str->length + new->length;

can be replaced by

++str->length;

str->length += new->length;

Trivial typo: sstream_lenght should be sstream_length.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.