Thread: Make a string out of part of a string

I have a string that contains a various number of lines which are each separated by \n and so what I want to do is to put each line into a node in a linked list.

The relevant sections in my code are as follows:

Code:

typedef struct line *Line;

struct line {
    char *text;
    int lineNum;
    Line next;
};

static Line newLines (char text[]) {
    Line list, curr = list;
	int lineCount = 1;
	int length = strlen(text);
	int subLength;
	int i=0, prevPos = i;
	
	list = malloc(sizeof(struct line));
	assert (list != NULL);
	list->text = NULL;
	list->lineNum = lineCount;
	list->next = NULL;
	
	for (; i<length; i++) {
	    if (text[i] == '\n') {
	        subLength = i-prevPos+1;
	        curr->text = malloc(sizeof(char)*(subLength+1));
	        assert (curr->text != NULL);
	        strncpy(curr->text, text[prevPos], subLength);
	        curr->text[subLength] = '\0';
	        
	        curr->next = malloc(sizeof(struct line));
	        curr = curr->next;
	        curr->text = NULL;
	        curr->lineNum = lineCount;
	        curr->next = NULL;
	        
	        prevPos = i+1;
	        lineCount++;
        }
	}
	
	if (lineCount == 1) {
	    curr->text = malloc(sizeof(char)*(length+1));
	    strcpy(curr->text, text);
	} else if (text[i] == '\0') {
        subLength = length-prevPos+1;
        curr->text = malloc(sizeof(char)*(subLength+1));
        assert (curr->text != NULL);
        strncpy(curr->text, text[prevPos], subLength);
    }

    return list;
}

In particular

Code:

strncpy(curr->text, text[prevPos], subLength);

With this line, I was hoping to make curr->text a string that is length subLength and begins at position prevPos in the text string. Except text[prevPos] is treated as a single character and not a string that begins at that position.

Originally Posted by c99tutorial

Just a guess, because I haven't tried your code. Maybe you want to do

&text[prevPos]

instead.

Oh right, I totally forgot about the ampersand.

Another comment: the typedef is not good in my opinion because it hides the pointer type. Normal way is to use a typedef such as

Code:

typedef struct Line Line;

struct Line {
...
};

Some people like to combine those two statements into one and remove the struct tag. So,

Code:

typedef struct {
...
} Line;

Either way now it is much clearer that this object needs to be allocated and freed

Code:

Line *line = malloc(sizeof(*line));

Also you should not use "assert" to test for successful allocation. Proper way is to use if

Code:

if ((line = malloc(sizeof(*line))) == NULL) {
   /* error occured */
}

Reason is because assert's are disabled by defining NDEBUG at compile time.

I'm all for learning better coding practises, unless it deviates from my professor's style. I appreciate your tips but changing it would just cause undue confusion.
Also, about the asserts being disabled, to be honest, I have never once encountered a situation where memory allocation failed, and the only reason I put the assert in there is because my professor does and I don't want him to have a fit when he sees me not using error checking (which apparently the assert achieves).

Due to popular criticism, I removed the asserts

I'm having a seg fault that I just don't understand...

Code:

//Compile: gcc -Wall -Werror -O -o text textbuffer.c tests.c

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

#include "textbuffer.h"

void initialize (void);

int main (int argc, char *argv[]) {
    initialize ();
    return EXIT_SUCCESS;
}

void initialize (void) {
    {
        TB TestSubject;
        printf("\nTest 1: Initialize with text \"abcd\"\n");   
        TestSubject = newTB("abcd");
        printf("Print text -\n%s\n", dumpTB(TestSubject));
        printf("Free text buffer\n");
        releaseTB(TestSubject);
        printf("Passed\n\n");
    }
    
    {
        TB TestSubject;
        printf("Test 2: Text = \"u\\nda\\nsexay \\nno homo\"\n");   
        TestSubject = newTB("u\nda\nsexay \nno homo");
        printf("Print text - \n%s\n", dumpTB(TestSubject));
        printf("Number of lines = %d\n", linesTB(TestSubject));
        printf("Free text buffer\n");
        releaseTB(TestSubject);
        //printf("Attempt to find numLines should result in error:\n");
        //printf("%d\n", linesTB(TestSubject));
        printf("Passed\n\n");
    }
    
    return;
}

Code:

//Date created: 09-Dec-13
//Date modified: 09-Dec-13

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

#include "textbuffer.h"

typedef struct line *Line;

struct line {
    char *text;
    int lineNum;
    Line next;
};

typedef struct textbuffer *TB;

struct textbuffer {
    Line first;
	int numLines;
};

static Line newLines (char text[]);
static void memoryError (void);

/* Allocate a new textbuffer whose contents is initialised with the text given
 * in the array.
 */
TB newTB (char text[]) {
    TB Buff;
	Line curr;
	int count = 0;
	
	if ((Buff = malloc(sizeof(struct textbuffer))) == NULL)
	    memoryError();
	
	Buff->first = newLines(text);
	for (curr = Buff->first; curr != NULL; curr = curr->next)
	    count++;
	
	Buff->numLines = count;
	
	return Buff;
}

static Line newLines (char text[]) {
    Line list = NULL, curr = list;
	int lineCount = 1;
	int length = strlen(text);
	int subLength;
	int i=0, prevPos = i;
	
	if (*text == '\0')
	    return NULL;
	
	if ((list = malloc(sizeof(struct line))) == NULL)
	    memoryError();
	assert (list != NULL);
	list->text = NULL;
	list->lineNum = lineCount;
	
	for (; i<=length; i++) {
	    if (text[i] == '\n') {
	        subLength = i-prevPos+1;
	        
            //line text:
	        if ((curr->text = malloc(sizeof(char)*(subLength+1))) 
                == NULL)
                memoryError();
	        assert (curr->text != NULL);
	        strncpy(curr->text, &text[prevPos], subLength);
	        curr->text[subLength] = '\0';
	        //line number:
	        curr->lineNum = lineCount;
	        //next line:
	        if ((curr->next = malloc(sizeof(struct line))) == NULL)
	            memoryError();
	        
	        curr = curr->next;
	        prevPos = i+1;
	        lineCount++;
        }
	}
	
    subLength = length-prevPos+1;
    if ((curr->text = malloc(sizeof(char)*(subLength+1))) == NULL)
        memoryError();
    strncpy(curr->text, &text[prevPos], subLength);
    
    curr->next = NULL;

    return list;
}


/* Free the memory occupied by the given textbuffer.  It is an error to access
 * the buffer afterwards.
 */
void releaseTB (TB tb) {
    Line curr = tb->first, temp;
    for (; curr != NULL; curr = curr->next) {
        temp = curr->next;
        free (curr->text);
        free (curr);
        curr = temp;
    }
    free(tb->first);
    free(tb);
    tb = NULL;
}

/* Allocate and return an array containing the text in the given textbuffer.
 */
char *dumpTB (TB tb) {
    Line curr;
    for (curr = tb->first; curr != NULL; curr = curr->next)
        printf("%s", curr->text);
        
    return NULL;
}


/* Return the number of lines of the given textbuffer.
 */
int linesTB (TB tb) {
    return tb->numLines;
}

static void memoryError (void) {
    fprintf(stderr, "Memory allocation failed\n");
    abort();
}

I get an error when I try to access a node's string, and I have no idea why.

Presumably if curr is NULL, curr->text causes the segfault. I say if, but curr is guaranteed to be NULL because you initialize it that way.

Derp... I've made that mistake in the past as well. It's just hard to override some common sense sometimes. The fact that I let list = NULL and then curr = list, and while I gave room to list on the heap, it's evidently tempting to think that curr == list continues to be true with the way I had written it.

So my problems up to this point are resolved. Till next time! (probably very soon)