Thread: segmentation error!

  1. #1
    Registered User
    Join Date
    Jan 2014
    Posts
    9

    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.
    Last edited by oceans; 07-28-2014 at 07:58 PM.

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,323
    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);
            }
        }
    }
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Jan 2014
    Posts
    9
    Quote Originally Posted by laserlight View Post
    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);
            }
        }
    }
    Thanks! While I'm no longer getting segmentation fault with your code its now printing just some random integer probably the sum.

    I just don't understand that why it wouldn't print the string part of " printf("%s %d\n", head->expression,sum);" ?.it works fine if I take out all the code for sum.

  4. #4
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,323
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  5. #5
    Registered User
    Join Date
    Jan 2014
    Posts
    9
    Quote Originally Posted by laserlight View Post
    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.
    Thanks! makes sense now. I didn't know that it modified the original string permanently.

  6. #6
    Registered User zub's Avatar
    Join Date
    May 2014
    Location
    Russia
    Posts
    104
    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;
    }
    Our goals are clear, tasks are defined! Let's work, comrades! -- Nikita Khrushchev

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,323
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    Registered User zub's Avatar
    Join Date
    May 2014
    Location
    Russia
    Posts
    104
    Quote Originally Posted by laserlight View Post
    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.
    Last edited by zub; 07-30-2014 at 11:12 AM.
    Our goals are clear, tasks are defined! Let's work, comrades! -- Nikita Khrushchev

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,323
    Quote 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.

    Quote 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).
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Registered User zub's Avatar
    Join Date
    May 2014
    Location
    Russia
    Posts
    104
    Have you tested your code?
    Yes.

    You should read up on the opaque pointer idiom before making that claim.
    May be.
    Our goals are clear, tasks are defined! Let's work, comrades! -- Nikita Khrushchev

  11. #11
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,323
    Quote Originally Posted by zub
    Yes.
    Then you must have missed at least one straightforward test case. For example, test with:
    Code:
    123 + 45 + 678=
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  12. #12
    Registered User zub's Avatar
    Join Date
    May 2014
    Location
    Russia
    Posts
    104
    Ah, I understand now. Yes, my code isn't very flexible. But you can imagine it as working prototype.
    Our goals are clear, tasks are defined! Let's work, comrades! -- Nikita Khrushchev

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. segmentation error
    By MASX in forum C Programming
    Replies: 6
    Last Post: 03-12-2011, 07:52 AM
  2. Segmentation Error
    By mrdibby in forum C Programming
    Replies: 4
    Last Post: 12-06-2009, 04:25 AM
  3. Segmentation Error / Bus Error in ANSI
    By drag0n69 in forum C Programming
    Replies: 10
    Last Post: 02-05-2008, 09:45 AM
  4. Segmentation error
    By zmaker5 in forum C Programming
    Replies: 5
    Last Post: 08-02-2007, 11:55 AM
  5. What is a segmentation error?
    By Crankit211 in forum C Programming
    Replies: 3
    Last Post: 12-11-2001, 01:08 AM