0

I'm having a small issue with my code for a linked list using a basic struct called element.

The program itself is pretty basic: input a text file and output each distinct word along with how many times the word occurs.

I have the output working exactly how I want it.

But I'm having trouble making sure all of the allocated memory is deleted at the end of my program.

Valgrind tells me 1 block of 48 bytes is still being occupied and the issue has something to do with line 39: **temp = new element;** but I still don't know if I'm failing to delete the first or the last element or where I should do that.

Any insight?

    std::string fileName;
    element * head = nullptr;
    element * temp = nullptr;

    // Input fileName to be read into file stream
    std::cout << "Input a file name to count the frequency of each word used: ";
    std::cin >> fileName;
    std::ifstream infile(fileName);
    if (!infile)
    {
            std::cout << "Error: File name [" << fileName << "] not found!" << std::endl;
            return 0;
    }

    // File stream inputs  text file into array, then takes it out if the word is already stored
    while(!infile.eof())
    {
            element * curr = nullptr;
            temp = new element;
            temp->next = nullptr;
            infile >> temp->word;

            if(head == nullptr){            // If list is empty, head becomes the first node
                    head = temp;
                    head->count++;
            } else {                        // but if the list isn't empty
                    curr = head;            // the current element goes inti the list
                    while (curr->next != nullptr){
                            if(curr->word == temp->word){   // If a duplicate it detected
                                    curr->count++;          // The count increases
                                    delete temp;
                                    temp = nullptr;         // and temp becomes null
                                    break;
                            }else{
                                    curr = curr->next;
                                    if(curr->word == temp->word){   // make sure to check the last element
                                            curr->count++;          // increase the count
                                            delete temp;
                                            temp = nullptr;         // and temp becomes null
                                            break;
                                    }
                            }
                    }
                    if(temp != nullptr && temp->word != ""){        // if none of those conditions are met
                    curr->next = temp;                              // then the new element gets put into the list
                    curr->next->count++;                            // and the count automatically goes to 1
                    }
            }
    }

    // Display the list using the current element istead of a for loop
    temp = head;
    while(temp != nullptr) {
            std::cout << "Word:     [ " <<  temp->word << " ] appears " << temp->count  << " time(s)." << std::endl;
            temp = temp->next;
    }

    // Delete memory allocated to the list
    while(head != nullptr) {
            temp = head->next;
            delete head;
            head = temp;
    }
    return 0;

}

Alex Yu
  • 2,366
  • 1
  • 20
  • 30
teabag5
  • 1
  • 2
  • 3
    Not sure if this is your problem, but you are not deleting ```temp``` if ```temp->word == ""``` evaluates to ```true``` and ```head == nullptr``` is ```false```. In that case none of your cases will apply and the pointer to ```temp``` will be overridden without deletion. – nick Feb 25 '19 at 00:11
  • Welcome to Stack Overflow. Please take a look at our [hlp section](https://stackoverflow.com/help), with special attention to the page on [minimal complete examples](https://stackoverflow.com/hlp/mcve). In this case, it would be helpful to pare the input file down to the simplest example that reproduces the error, then hard-code it into the code and post *that.* – Beta Feb 25 '19 at 00:12
  • @nick I tried what you said with the code: if(temp->word == ""){ delete temp; temp = nullptr; } if(temp != nullptr){ curr->next = temp; curr->next->count++; } But now it's giving me a segmentation fault – teabag5 Feb 25 '19 at 01:01
  • 1
    @teabag5 I would suggest restructuring your code so you never actually create a new object unless you need it. Only in the last block ```if(temp != nullptr && temp->word != "")``` do you actually need a new object. Move the new object creation to that block, and just use a ```std::string``` for the rest. – nick Feb 25 '19 at 01:25
  • @nick I've tried putting the new element declaration in the last if block as well as the head == nullptr if block and replaced temp->word with a std::string tempWord variable for the comparison and value checks, but I'm still getting a segmentation fault – teabag5 Feb 25 '19 at 02:28
  • @teabag5 Not sure where the problem could come from in that case, sorry. Perhaps you should take Beta's advice and post your input, that might have a clue as to what's going wrong. – nick Feb 25 '19 at 03:22
  • I can hardly imagine ```while(!infile.eof())``` would cause a segfault, but using ```eof()``` is generally considered to be wrong, maybe that's causing a problem https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong – nick Feb 25 '19 at 03:25
  • Ahh I found a solution. Initialize head to new element instead of nullptr, then replace temp = head; before the display loop with the code: temp = head->next; delete head; head = temp; and now it works with no memory leaks. – teabag5 Feb 25 '19 at 04:10
  • Creating a smart pointer ala `std::unique_ptr` would avoid raw `new`/`delete`. – Jarod42 Feb 25 '19 at 08:56

0 Answers0