Memory not being freed properly

Question

I am wokring on my Hash Table assigment where a huge chunk of data is being read (from a csv file) into the Hash table. A requirement for this assignment is to use raw pointers (no #include <memory> library either) to the DataEntry class. (although I hate that, I still have to follow it)

I got it covered for the most part and the program works, but when I run a tool like valgrind:

valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --num-callers=20 --log-file=valgrind.log ./main

it shows I lost:

==4044056== LEAK SUMMARY:
==4044056==    definitely lost: 17,511,840 bytes in 243,220 blocks
==4044056==    indirectly lost: 1,257,494 bytes in 39,329 blocks
==4044056==      possibly lost: 391 bytes in 6 blocks
==4044056==    still reachable: 20,626 bytes in 292 blocks
==4044056==         suppressed: 0 bytes in 0 blocks

This is my CovidDB class where the Hash Table is being represented by a 2D vector due to the use of separate chaining to avoid colisions:

  private:
    int size = 17;
    std::vector<std::vector<DataEntry*>> HashTable;

  public:
    inline CovidDB() {
        HashTable.resize(size);
    }

    inline ~CovidDB() {
    for (auto& row : HashTable) {
      for (auto& entry : row) {
        delete entry;
      }
      row.clear();
    }
    HashTable.clear();

    HashTable.shrink_to_fit();
    std::cout << "IN HERE" << std::endl;
    }

I did some research on how to delete memory from 2D vectors and found that clear() removes the elements and shrink_to_fit() deals with the memory. After changing that I was still leaking a lot of memory so I figured my destructor is not being called.

I put a cout statement in the destructor to see if its even being called. "IN HERE" is not being printed to the terminal at all.

My professor requiers us to only have 1 function in our main.cpp file:

#include "CovidDB.h"
#include "run.h"

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

Could that be the origin of the error? Is there a different way to handle memory? How do I explicitly call the destructor? I tried:

CovidDB dataBase;
dataBase.~CovidDB();

And I get an error saying there is not matching operator.

Here is my add function that allocated the memory:

bool CovidDB::add(DataEntry* entry) {
    time_t now = time(0);
    tm* ltm = localtime(&now);
    // DATE FORMAT: mm/dd/yy
    std::string current_date_str = std::to_string(1 + ltm->tm_mon) + "/" + std::to_string(ltm->tm_mday) + "/" + std::to_string(ltm->tm_year % 100);
    std::istringstream iss(current_date_str);
    std::tm current_date = {};
    iss >> std::get_time(&current_date, "%m/%d/%y");

    std::tm entry_date = {};
    std::istringstream iss2(entry -> get_date());
    iss2 >> std::get_time(&entry_date, "%m/%d/%y");

    if (mktime(&current_date) > mktime(&entry_date)) {
        std::cout << "[Record rejected]" << std::endl;
        return false;
    }

    int index = hash(entry -> get_country());
    assert(index < 17 and index >= 0);

    if (HashTable[index].empty()) {
        HashTable[index].push_back(new DataEntry(*entry));
    } else {
        bool added = false;
        for (DataEntry* existing_entry : HashTable[index]) {
            if (hash(existing_entry->get_country()) == hash(entry->get_country()) &&
                existing_entry->get_country() == entry->get_country()) {
                existing_entry->set_c_cases(existing_entry->get_c_cases() + entry->get_c_cases());
                existing_entry->set_c_deaths(existing_entry->get_c_deaths() + entry->get_c_deaths());
                added = true;
                break;
            }
        }
        if (!added) {
            HashTable[index].push_back(new DataEntry(*entry));
        }
    }

    return true;
}

Here is my 'main' function that is being called in main

void set_data(DataEntry* data, std::string country, std::string date, int cases, int deaths) {
  data->set_country(get_country(country));
  data->set_date(get_date(date));
  data->set_c_cases(get_covid_cases(cases));
  data->set_c_deaths(get_covid_deaths(deaths));
}

void run() {
  /** DECLARATIONS: */
  std::cout << std::endl << std::endl << std::flush;
  CovidDB dataBase;
  DataEntry *data = new DataEntry;

  std::string country = "\n";
  std::string date = "\0";
  int cases = 0;
  int deaths = 0;

  /** DECLARATIONS: */


  while(true) {
    menu();
    switch(get_input()) {
    case OPID::ID_1: {
      dataBase.add_covid_data(COVID_FILE);
      break;
    }
      
    case OPID::ID_2: {
      set_data(data, country, date, cases, deaths);
      dataBase.add(data); //fix
      break;
    }
      
    case OPID::ID_3: {
      dataBase.get(get_country(country));
      break;
    }
      
    case OPID::ID_4: {
      dataBase.remove(get_country(country));
      break;
    }
      
    case OPID::ID_5: {
      dataBase.display_table();
      break;
    }
      
    case OPID::ID_6: {
      goodbye();
      std::exit(EXIT_SUCCESS);
      break;
    }
      
    default: {
      /** @note do nothing...*/
    }
    }
  }
  std::cout << std::endl << std::endl << std::flush;
}

Makefile:

CXX = g++
CXXFLAGS = -std=c++11 -Wall -Werror -pedantic -O2

SRCS = run.cpp CovidDB.cpp main.cpp
OBJS = $(SRCS:.cpp=.o)
EXEC = main

.PHONY: clean

all: $(EXEC)

$(EXEC): $(OBJS)
    $(CXX) $(CXXFLAGS) $(OBJS) -o $(EXEC)

%.o: %.cpp
    $(CXX) $(CXXFLAGS) -c $< -o $@

clean:
    rm -f $(OBJS) $(EXEC)

The project itself is big and I don't want to paste everything in here. If some other functions I missed are requiered for further analasys plese let me know.

Thank you.

I tried changing the destructor and I tried debugging it, but that only shows me where memory is being allocated. I though I can delete the whole hash table in the destructor but I guess not.

Answer 1

I see you have a destructor that is deleting hash entries that have been allocated. However, you are performing redundant allocation and have no code that is deleting hash entries that were rejected in the 'add' function

HashTable[index].push_back([ new DataEntry(*entry) );

The memory that the pointer refers to has already been allocated. Calling new again will not transfer the memory of your pointer to a new address. It will instead allocate a different pointer to a different 'entry'. You will end up with two identical 'entries' at different addresses

Also be sure to delete the pointer if the destructor won't be the one cleaning up the allocated memory.

//In add
hashTable[index].push_back(entry);
//...
if(!added)
{
    //...
    return false;
}


//In run
//...
case OPID::ID_2: {
set_data(data, country, date, cases, deaths);
if(!dataBase.add(data))
{
    delete data;
}
break;
//...