1

I am currently working on a text editing program in C, which uses Linked Lists for rows of text. I have so far written functions for resizing the list etc., but I have now attempted to write the insert_char(Row* row, int idx, char c) however whenever I try resizing it, the resulting char* array is NULL. I am confident it's not a memory leak, as I have checked and I am free()ing all of my malloc()'d memory, so I really don't know where the problem is.

I have also tried some printf("%c", c) debugging to view the character, however the character itself is also NULL. Can anyone help me with this?

Here is the struct for a Row:

typedef struct {
    char* data; // pointer to Malloc()'d char array.
    int datalen;
} Row;

Here are the functions for resizing the row and allocating the Row pointer.

Row* alloc_row(char* data)
{
    Row* row = (Row*) malloc(sizeof(Row));
    char* data2 = (char*) malloc((sizeof(char) * strlen(data))+1);
    strcpy(data2, data);
    row->data = data2;
    row->datalen = strlen(data);
    return row;
}

// Row resize

Row* resize_row(Row* oldrow, char* data)
{
    Row* new_row = (Row*) malloc(sizeof(Row));
    new_row->data = data;
    new_row->datalen = strlen(data);

    // free() the old row
    free(oldrow->data);
    free(oldrow);

    return new_row;
}

And here is the function I am having trouble with - it should take a Row*, create a buffer, strcpy() the Row->data up to idx, insert the char c and then copy the rest of the string afterwards, such that if I called alloc_row(Row* {.data = "Hello" .strlen=5}, 2, 'A') I would receive HeAllo (counting from zero). However, the string is NULL:

Row* insert_char(Row* row, int idx, char c)
{
    char* new_row = (char*)malloc(sizeof(char) * (strlen(row->data) + 2)); // 1 char for null, char for the appended data
    if (idx < strlen(row->data)) {
        for (int i = 0; i < strlen(row->data)+1; i++) {
            if (i < idx) new_row[i] = row->data[i];
            if (i == idx) new_row[idx] = c;
            if (i > idx) new_row[i] = row->data[i-1];
        }
    } else {
        row->data[strlen(row->data)] = '\0';
        strncpy(new_row, row->data, strlen(row->data));
        new_row[strlen(row->data)-1] = c;
    }
    Row* nr = resize_row(row, new_row);
    return nr;
}

Is there something wrong with my approach, and is there a cleaner and faster way of doing this?

9
  • 3
    row->data[strlen(row->data)] = '\0'; makes no sense as strlen uses the nul termination char to compute the length of the string Commented Jan 6, 2022 at 7:54
  • new_row->data = data; then free(oldrow->data); that's not okay: you're copying the input pointer and freeing the old memory block. You should duplicate the input string instead Commented Jan 6, 2022 at 7:56
  • 1
    All those duplicated strlen calls - so wasteful ! Commented Jan 6, 2022 at 7:57
  • 1
    That’s the reason the code is so untidy, as I wanted no room for error, and then I planned on going through to optimize later. I do appreciate it is an eyesore Commented Jan 6, 2022 at 7:58
  • 1
    It allocates a memory for a new Row and assigns some pointers. It doesn't resize anything. You'd be better off using alloc_row and providing a new function to free a row. Commented Jan 6, 2022 at 8:14

3 Answers 3

1

At least these problems:

Not a string

new_row[] is not a string as it lacks a null character. Later code relies on that.
Result: undefined behavior (UB).

char* new_row = (char*)malloc(sizeof(char) * (strlen(row->data) + 2));
if (idx < strlen(row->data)) {
  ...
} else {
    row->data[strlen(row->data)] = '\0';
    strncpy(new_row, row->data, strlen(row->data));
    // At this point `new_row[]` lacks a '\0'
    new_row[strlen(row->data)-1] = c;
}

It is unclear exactly what OP's wants in the else block, but I think it may be:

} else {
    size_t len = strlen(row->data);
    strcpy(new_row, row->data);
    new_row[len++] = c;
    new_row[len] = '\0';
}

Minor: conceptually wrong size

The below works OK because (sizeof(char) is 1.

char* data2 = (char*) malloc((sizeof(char) * strlen(data))+1);

But should be:

char* data2 = (char*) malloc(sizeof(char) * (strlen(data) + 1));

Even better, drop the unneeded cast and size to the referenced object, not the type.

char* data2 = malloc(sizeof *data2 * (strlen(data) + 1u));
// or
char* data2 = malloc(sizeof data2[0] * (strlen(data) + 1u));

Untested alternate code

typedef struct {
  char *data; // pointer to Malloc()'d char array.
  //int datalen;
  size_t datalen;
} Row;

// Row* insert_char(Row *row, int idx, char c) {
Row* insert_char(Row *row, size_t idx, char c) {
  assert(c != 0);  // Unclear what OP wants in this case

  //char *new_row = (char*) malloc(sizeof(char) * (strlen(row->data) + 2));
  // Why use strlen(row->data) when the length is in row->datalen ?
  // Since row->data was getting free'd later in OP's code,
  // let us just re-allocate instead and re-use the old row node.
  char *new_row = realloc(row->data, row->datalen + 2);
  assert(new_row); // TBD code to handle out-of-memory

  // When idx large, simply append
  if (idx > row->datalen) {
    idx = row->datalen;
  }

  // Shift the right side over 1
  memmove(new_row + idx + 1, new_row + idx, row->datalen - idx + 1);  // Moves \0 too
  new_row[idx] = c;

  row->data = new_row;
  row->datalen++;
  return row;
}
Sign up to request clarification or add additional context in comments.

Comments

0

I tried the following code and it works (I modified certain things to print it directly and corrected some of your suggestions on how to call the function):

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

typedef struct {
    char* data; // pointer to Malloc()'d char array.
    int datalen;
} Row;

char* insert_char(Row* row, int idx, char c)
{
    char* new_row = (char*)malloc(sizeof(char) * (strlen(row->data) + 2)); // 1 char for null, char for the appended data
    if (idx < strlen(row->data)) {
        for (int i = 0; i < strlen(row->data)+1; i++) {
            if (i < idx) new_row[i] = row->data[i];
            if (i == idx) new_row[idx] = c;
            if (i > idx) new_row[i] = row->data[i-1];
        }
    } else {
        row->data[strlen(row->data)] = '\0';
        strncpy(new_row, row->data, strlen(row->data));
        new_row[strlen(row->data)-1] = c;
    }
    return new_row;
}


int main()
{
    printf("%s\n", insert_char(&(Row) {.data = "Hello", .datalen=5}, 2, 'A'));

    return 0;
}

However, I think that your problem is in the for where you need +2 instead of +1 in the ending condition (since you are copying the entire array and malloc doesn't necessarly set the last char as '\0' [although calloc could do that]).

2 Comments

Minor: int idx, int i, better as size_t to match the type returned by strlen().
Well, actually I would change more things, but I tried to keep everything as similar as the code from the question, adding just my suggestion at the end.
0

Using some of your great ideas, I have come up with the following sample which uses calloc() to initialise a section of memory to 0. I believe my issue was in fact a missing NULL byte, and I have also cleaned things significantly. Here is my improved snippet:

Row* insert_char(Row* row, int idx, char* str)
{
    char* new_row = calloc(row->datalen + strlen(str) + 1, sizeof(char));
    strncpy(new_row, row->data, idx);
    strcat(new_row, str);
    strcat(new_row, row->data + idx);

    return resize_row(row, new_row);
}

NOTE: I have modified the input from a char to a char* because I plan to be inserting strings in the future, and not just single characters.

The same resize_row() method is used as in the original:

Row* resize_row(Row* oldrow, char* data)
{
    Row* new_row = (Row*) malloc(sizeof(Row));
    new_row->data = data;
    new_row->datalen = strlen(data);

    // free() the old row
    free(oldrow->data);
    free(oldrow);

    return new_row;
}

2 Comments

strcat(new_row, str); strcat(new_row, row->data + idx); uses the inefficient Schlemiel the Painter's Algorithm. Instead, strcpy(new_row + idx, str); strcat(new_row + idx, row->data + idx);
Why are you allocating a Row and then freeing a Row? Better to use the Row over again and skip the unneeded malloc(sizeof(Row)); ... free(oldrow);.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.