0

I've moved ahead from my segfault in load (i think) (see previous post here)

But now my code seems to run an infinite loop which am pretty sure still originated from the load (as debug50 does not jump into load in dictionary.c when hit line 45 in speller.c (bool loaded = load(dictionary).

below is my 'load' codes

bool load(const char* dictionary)
{
    // open dictionary
    FILE* dic = fopen(dictionary, "r");

    // if unsuccessful do not continue
    if(dic == NULL)
    {
        printf("Could not open %s.\n", dictionary);
        return false;
    }

    // allocate memory to root
    for(int i = 0; i < ALPHABETS; i++)
    {
        root = NULL;
        root = (node*)calloc(1, sizeof(node));
    }

    // if unsuccessful do not continue
    if(root == NULL)
    {
        printf("calloc failed\n");
        return false;
    }

    // initiate trav pointer
    trav = root;

    // read each dictionary word letter by letter and record in trie
    for(int key = fgetc(dic); key != EOF; key = fgetc(dic))
    {
        while(key != '\n')
        {
            if(isalpha(key))
            {
                if(trav->children[key - 'a'] == NULL)
                {
                    trav->children[key - 'a'] = (node*)calloc(1, sizeof(node));

                    // if unsuccessful  
                    if(trav->children[key - 'a'] == NULL)
                        return false;

                    trav = trav->children[key - 'a'];
                    trav->is_word = false;
                    key++;
                }
            }

            if(key == '\'')
            {
                 if(trav->children[25] == NULL)
                {
                    trav->children[25] = (node*)calloc(1, sizeof(node));

                    // if unsuccessful  
                    if(trav->children[25] == NULL)
                        return false;

                    trav = trav->children[25];
                    trav->is_word = false;
                    key++;
                }
            }

            if(key == '\0')
            {
                 if(trav->children[26] == NULL)
                {
                    trav->children[26] = (node*)calloc(1, sizeof(node));

                    // if unsuccessful  
                    if(trav->children[26] == NULL)
                        return false;

                    trav = trav->children[26];
                    trav->is_word = true;
                    trav = root;
                    key++;
                }
            }
        }
        if(key == EOF)
            return true;
    }
    // close source file
    fclose(dic);
    return false;
}

Any help is greatly appreciated. Ta

Edited code (the one got killed by server coz it hit memory limit)

bool load(const char* dictionary)
{
    // open dictionary
    FILE* dic = fopen(dictionary, "r");

    // if unsuccessful do not continue
    if(dic == NULL)
    {
        printf("Could not open %s.\n", dictionary);
        return false;
    }

    // allocate memory to root
    for(int i = 0; i < ALPHABETS; i++)
    {
        root = NULL;
        root = (node*)calloc(1, sizeof(node));
    }

    // if unsuccessful do not continue
    if(root == NULL)
    {
        printf("calloc failed\n");
        return false;
    }

    // initiate trav pointer
    trav = root;

    // read each dictionary word letter by letter and record in trie

    char word[LENGTH+1];
    int index = 0;
    int words = 0; 

    for(int key = fgetc(dic); key != EOF; key = fgetc(dic))
    {
        while(key != '\n')
        {
            if(isalpha(key) || (key == '\'' && index > 0))
                word[index] = key;
                index++;
            if(key == '\0')
                word[index] = '\0';
                words++;

            for(int l = 0, m = strlen(word); l < m; l++)
            {
                if(word[l] == isalpha(key))
                    trav = trav->children[key - 'a'];

                if(word[l] == '\'')
                    trav = trav->children[25];

                if(word[l] == '\0')
                    trav = trav->children[26];

                for(int i = 0; i < ALPHABETS; i++)
                {
                    trav = trav->children[i];
                    trav = NULL;
                    trav = (node*)calloc(1, sizeof(node));
                }

                // if unsuccessful do not continue
                if(trav == NULL)
                {
                    printf("calloc failed\n");
                    return false;
                }
            }
        }
        trav->is_word = true;
        return true;
        key++;
    }
    // close source file
    fclose(dic);
    return false;
}
4
  • I really can't understand this. The changes I've made in the previous post at first pass the segfault when I run the code, but now the segfault is back again, when I tried to tweak the code above. It still the same line "if(trav->children[key - 'a'] == NULL)"...It really frustrates me!!
    – MTreacy
    Nov 9 '16 at 3:50
  • Heyy!! 've managed to move pass the above segfault. had a couple of segfault afterwards, but then seems to have passed it with load trying to run (at least processing something). Unfortunately it hit the memory limit and got killed by the server. :-( When I tried to run with small dictionary, it gives this error "could not open&load dictionary/small". Any idea why??
    – MTreacy
    Nov 9 '16 at 11:27
  • So which code should I look at? The first code block or the second one? Nov 9 '16 at 15:07
  • Sorry, the second one, please
    – MTreacy
    Nov 9 '16 at 21:46
1

Written based on the second code:

You are allocating multiple times, overwriting the pointer each time? valgrind won't like that:

    for(int i = 0; i < ALPHABETS; i++)
    {
        root = NULL;
        root = (node*)calloc(1, sizeof(node));
    }

I think you should just do

    root = (node*)calloc(1, sizeof(node));

I don't like the while-loop inside the for-loop. You probably should get rid of it.

Words are not terminated by null-bytes in the file, but by line breaks, that should trigger writing a '\0' and running the inner for-loop.

Maybe you could have some additional check if index is still in range, and if not, reset index and skip all characters until after '\n' or reaching EOF.

Indentation is misleading:

        if(isalpha(key) || (key == '\'' && index > 0))
            word[index] = key;
            index++;

actually means

        if(isalpha(key) || (key == '\'' && index > 0))
            word[index] = key;
        index++;

while you mean

        if(isalpha(key) || (key == '\'' && index > 0))
        {
            word[index] = key;
            index++;
        }

On '\'', you should access element 26, not 25 (which is z!)

'\0' won't happen, as you loop only the string characters. After the inner for-loop (still only on encountering '\n' in input), the code should change is_word and reset trav to root and index to 0.

Instead of just

                    trav = trav->children[26];

you first should make sure that one is not NULL:

                {
                    if (!trav->children[26])
                    {
                        trav->children[26] = (node*)calloc(1, sizeof(node));
                    }
                    trav = trav->children[26];
                }

(same for other lines like that in the load function)

Again, I don't understand the loop where you allocate something, just to throw away the pointer.

Don't return true anywhere in the loop (maybe return false on error), and return true as the last thing in the function.

key++ does not make any sense to me (you increment the character, for example from 'a' to 'b')

2
  • Wow... Thanks for the suggestions @Blauelf. I'll look into this asap and get back to you. :-)
    – MTreacy
    Nov 9 '16 at 21:49
  • Thanks again, @Blauelf!! After changing some codes based on your suggestion, I've now managed to pass load and move on to check. I still have some work to do there, but couldn't have done it without you...
    – MTreacy
    Nov 9 '16 at 22:46

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .