Thread: segmentation error!

I need some help here.

basically i have a linked list of strings of the following format.

"integer operator integer ...="

it will contain atleast 2 integers but can be of varied length.

head->expression contains the string. I'm trying to calculate the sum and print it along the string.

Code:

void printQueue(struct node *head)
{
        char *del = " =";
        char* token;
        char* op;
        int sum = 0;
        while(head != NULL)
        {


                token = strtok(head->expression,del);
                while(token!=NULL)
                {
                        op = strtok(NULL,del);


                        if (strcmp(op,"+")==0)
                        {
                                sum = atoi(token) + atoi(strtok(NULL,del));
                        }
                        else if (strcmp(op,"-") == 0)
                        {
                                sum = atoi(token) - atoi(strtok(NULL,del));
                        }




                }
        printf("%s %d\n", head->expression,sum);
        head = head->next;
        sum = 0;
}
}

I'm getting seg fault or just plain blank output.

works fine if i just print the strings.

You should be more consistent with your indentation:

Code:

void printQueue(struct node *head)
{
    char *del = " =";
    char* token;
    char* op;
    int sum = 0;
    while(head != NULL)
    {
        token = strtok(head->expression,del);
        while(token!=NULL)
        {
            op = strtok(NULL,del);
            if (strcmp(op,"+")==0)
            {
                sum = atoi(token) + atoi(strtok(NULL,del));
            }
            else if (strcmp(op,"-") == 0)
            {
                sum = atoi(token) - atoi(strtok(NULL,del));
            }
        }
        printf("%s %d\n", head->expression,sum);
        head = head->next;
        sum = 0;
    }
}

Looking at your string format, I suggest a slightly different approach, i.e., start with the first operand, then parse the operator and next operand together:

Code:

token = strtok(head->expression, del);
if (token)
{
    sum = atoi(token);
    while ((op = strtok(NULL, del)) && (token = strtok(NULL, del)))
    {
        if (strcmp(op, "+") == 0)
        {
            sum += atoi(token);
        }
        else if (strcmp(op, "-") == 0)
        {
            sum -= atoi(token);
        }
    }
}

The problem is that strtok modifies the string. If you want to print the original string, then you should make a copy of it and use strtok on that copy.

Code:

#include <stdlib.h>     // malloc, strtol
#include <string.h>     // strpbrk
#include <stdio.h>      // fgets


typedef struct Expression {
    int x;
    char op;
    int y;
} *Expression;


Expression Expression_New(char* str)
{
    Expression new = (Expression) malloc(sizeof(struct Expression));
    new->op = '\0'; // error
    char* end;
    new->x = strtol(str, &end, 10);
    if( end == str ) { return new; }
    str = strpbrk(end, "+-/*");
    if( !str ) { return new; }
    new->op = *str;
    new->y = strtol(++str, &end, 10);
    if( end == str ) { new->op = '\0'; }
    return new;
}


int main(void)
{
    char buf[20];
    char* str;
    while( str = fgets(buf, sizeof(buf), stdin) ) {
        Expression ex = Expression_New(str);
        if( ex->op ) {
            printf("%d %c %d\n", ex->x, ex->op, ex->y );
        }
        free(ex);
    }
    return 0;
}

zub's example will not handle the "it will contain atleast 2 integers but can be of varied length" aspect of the input format, but I suggest that you take a look at the use of strtol as an alternative to atoi that provides proper error checking.

What I recommend against is the typedef of a pointer. As the opaque pointer idiom is not in use, it is confusing since you have statements like:

Code:

Expression ex = Expression_New(str);
if( ex->op ) {

where it does not look like ex is a pointer yet ex is used as a pointer.

Originally Posted by laserlight

zub's example will not handle the "it will contain atleast 2 integers but can be of varied length"

Code:

if( end == str || new->x < 10 ) { return new; } // Line 19
if( end == str || new->y < 10 ) { new->op = '\0'; } // Line 24

What I recommend against is the typedef of a pointer. As the opaque pointer idiom is not in use, it is confusing since you have statements like:

In my code style all uppercase types are opaque pointers.

Originally Posted by zub

Code:

if( end == str || new->x < 10 ) { return new; } // Line 19
if( end == str || new->y < 10 ) { new->op = '\0'; } // Line 24

Have you tested your code? I have, hence I am confident that my assertion is correct. The reason is that your code reads line by line with fgets, parsing that line for a single binary expression.

Originally Posted by zub

In my code style all uppercase types are opaque pointers.

You should read up on the opaque pointer idiom before making that claim. If indeed Expression were an opaque pointer type, then ex->op in the main function would result in a compile error (unless the programmer deliberately circumvented the idiom, but that is not always feasible).

Have you tested your code?

Yes.

You should read up on the opaque pointer idiom before making that claim.

May be.

Originally Posted by zub

Yes.

Then you must have missed at least one straightforward test case. For example, test with:

Code:

123 + 45 + 678=

Ah, I understand now. Yes, my code isn't very flexible. But you can imagine it as working prototype.