C++ Help? Dynamic Memory Allocation Issues

[duckypath]

I am having trouble finding where the memory issue is, even after trying multiple ways, including not even freeing the memory. Any help appreciated! A lot of lines, so feel free to download the file. Can paypal someone $10 if you help me out here. Just point me in the right direction

 

ERROR: zsh: segmentation fault 

 

Sometimes I can get it to compile and finish the rest of main, but other times it won't. SO definitely undefined behavior, memory issue.

 

MAIN.CPP

#include <iostream>
#include "Set.h"
using namespace std;

int main()
{
    int arr1[6] = {2, 1, 4, 3, 5, 4};
    cout<<"int arr1[] = {2, 1, 4, 3, 5, 4}"<<endl;
    Set set1(arr1,6);
    cout << "Set1(arr1): ";
    set1.display();
    cout << endl;

    cout<<"adding 1, 3, 5 again then 6 and 7"<<endl;
    set1.add(1);
    set1.add(3);
    set1.add(5);
    set1.add(6);
    set1.add(7);

    cout << "Set1: ";
    set1.display();

    // remove an existing element and a non-existing element
    cout<<"Remove(7); Remove(9)"<<endl;
    set1.Remove(7);
    set1.Remove(9);
    cout << "Set1: ";
    set1.display();
    cout<<endl;

    cout << endl;
    cout << "Creating set2, copying the former set. (Testing copy constructor)" << endl;
    Set set2(set1);
    cout << "Set2: ";
    set2.display();
    cout << endl;

    int arr3[4] = {0, 1, 4, 9};
    Set set3(arr3, 4);
    cout << "Set3: ";
    set3.display();
    cout << endl;


    cout << "The union of set1 and set3 is: " << endl;
    Set unionSet = set1.Union(set3);
    unionSet.display();
    cout << endl;

    cout << "intersectSet = set1.Intersection(set3)" << endl;
    Set intersectSet = set1.Intersection(set3);
    intersectSet.display();
    cout << endl;

    // Test - the difference between two sets
    cout << "diffSet = unionSet.Difference(intersectSet)" << endl;
    Set diffSet = unionSet.Difference(intersectSet);
    diffSet.display();
    cout << endl;

    // Test == if two sets are equal
    Set set4(set1);
    cout << "Set4(set1): ";
    set4.display();
    cout << endl;

    cout << "set1: ";
    set1.display();
    cout << "set4: ";
    set4.display();
    if (set1.Equal(set4))
        cout<<"set1 is equal to set4"<<endl;
    else
        cout<<"set1.Equal(set4) failed"<<endl;
    cout << endl;

    // Test != if two sets are not equal
    cout << "set1: ";
    set1.display();
    cout << "set3: ";
    set3.display();
    if (!(set1.Equal(set3)))
        cout<<"set1 is not equal to set3"<<endl;
    else
        cout<<"!(set1.Equal(set3 ) failed"<<endl;
    cout << endl;

    return 0;
}

SET.H

#ifndef SET_H
#define SET_H


class Set
{
    private:
      int *arp;
      int pSize;
      int numElements;
      const static int DEFAULT_SIZE = 5;

    public:
      Set(int arrSize = DEFAULT_SIZE);
      Set(int[], int arrSize);
      ~Set();
      Set(const Set &origObj); // copy constructor
      //Set(Set *copyObj);
      
      void display();
      bool add(int newElement);
      bool isElement(int elementToCheck);
      void extendSet();
      void showSize();
      void countElements();
      bool Remove(int elementToRemove);
      Set Union(Set setToUnion);
      Set Intersection(Set setToIntersect);
      Set Difference(Set toDifference);
      bool Equal(Set equalCheckSet);
    
};

#endif

SET.CPP

#include <iostream>
#include "Set.h"


// Default constructor
Set::Set(int SIZE)
{
    pSize = SIZE;
    arp = new int[pSize];
    numElements = 0;
}

// Copy constructor
Set::Set(const Set &origObj)
{
  arp = origObj.arp;
  pSize = origObj.pSize;
  numElements = origObj.numElements;
}
/*
Set:: Set(Set *copyObj)
{
    for(int i = 0; i < copyObj->numElements; i++)
    {
      arp[i] = copyObj->arp[i];
    }
    numElements = copyObj->numElements;
    pSize = copyObj->pSize;
}
*/

// Constructor to create array and set its size.
Set::Set(int arr[], int arrSize)
{
  pSize = Set::DEFAULT_SIZE;
  arp = new int[pSize];
  for (int i = 0; i < arrSize; ++i)
  {
    add(arr[i]); // must add all the elements to arr.
  }   
}

// Destructor: removes the dynamically allocated array.
Set::~Set() 
{
    delete [] arp;
}

void Set::display()
{
  std::cout << "{";
  for (int i = 0; i < numElements; ++i)
  {
    std::cout << arp[i];
    if (i != numElements - 1) // checks for the last elm.
    std::cout<<", ";
  }
  std::cout << "}" << std::endl;

  /* // OLD, didnt end up working
    int count = 0;
    
    std::cout << "{";
    for (int i = 0; i < this->pSize; ++i)
    {
        if (i == this->pSize - 1)
        {
            std::cout << arp[i];
            break;
        }
        else if (this->arp[this->pSize - 1] < 1) {std::cout << ""; break;}

        std::cout << this->arp[i] << ", ";
    }
    std::cout << "}" << std::endl;
    */
}

bool Set::add(int newElement)
{
   if (!isElement(newElement))
   {
     if (numElements == pSize)
     {
       extendSet();
     }
     arp[numElements++] = newElement;
   }
   return false;
}

// checks of the input element is in the array
bool Set::isElement(int elementToCheck)
{
    for (int i = 0; i < numElements; ++i)
    {
        if (elementToCheck == this->arp[i])
        {
            return true;
        }
    }
    return false;
}

void Set::extendSet()
{
  //dynamically creating new arr in memory equal to new size + old default.
    int *newArr = new int[pSize + Set::DEFAULT_SIZE];
    pSize += Set::DEFAULT_SIZE;
    for(int i = 0; i < numElements; i++)
    {
      newArr[i] = arp[i];
    }
    arp = newArr;
}

// Shows the size of the current array
void Set::showSize()
{
    std::cout << "Current size of array: " << this->pSize << std::endl;
}

// will count current elements in array
void Set::countElements()
{
  std::cout << "Current Element Count: ";
  std::cout << numElements << std::endl;
}

bool Set::Remove(int elementToRemove)
{
  int elmRemovePosition;
  if (isElement(elementToRemove) == true)
  {
    for (int i = 0; i < pSize; ++i)
    {
      if (elementToRemove == arp[i])
      {
        elmRemovePosition = i;
      }

      for (int i = elmRemovePosition; i < numElements; ++i)
      {
        arp[i] = arp[i + 1];
      }
    }
  numElements--; // reduce array size by 1
  }
  else
  {
    return false;
  }
  return 0;
}

Set Set::Union(Set setToUnion)
{
  Set unionSet;
  for (int i = 0; i < numElements; ++i)
  {
    unionSet.add(arp[i]);
  }
  for (int i = 0; i < numElements; ++i)
  {
    unionSet.add(setToUnion.arp[i]);
  }
  return unionSet;
}

Set Set::Intersection(Set setToIntersect){
    Set intersectSet;
    for (int i = 0; i < numElements; i++){
        if(setToIntersect.isElement(arp[i]) != false) intersectSet.add(arp[i]);
    }
    return intersectSet;
}

Set Set::Difference(Set toDifference){
    Set differenceSet(toDifference);
     for (int i = 0; i < toDifference.numElements; i++)
     {
        differenceSet.Remove(toDifference.arp[i]);
     }
    return differenceSet;
}

/*
Set Set::Difference(Set toDifference)
{
  Set differenceSet;
  int tempElement;
  for (int i = 0; i < this->numElements; ++i)
  {
    for (int j = 0; j < toDifference.numElements; ++j)
    {
      if (arp[i] != toDifference.arp[j])
      {
        differenceSet.add(arp[i]);
      }
      else
      {
        tempElement = this->arp[i];
      }
    }
    differenceSet.Remove(tempElement);
  }
  return differenceSet;
}*/

/*
Set Set::Intersection(Set setToIntersect)
{
  Set intersectSet;
  for (int i = 0; i < this->numElements; ++i)
  {
    for (int j = 0; j < setToIntersect.numElements; ++j)
    {
      if (arp[i] == setToIntersect.arp[j])
      {
        intersectSet.add(arp[i]);
      }
    }
  }
  return intersectSet;
}*/

bool Set::Equal(Set equalCheckSet)
{
  if (numElements == equalCheckSet.numElements)
  {
    for (int i = 0; i < numElements; ++i)
    {
      for (int j = 0; j < equalCheckSet.numElements; ++j)
      {
        if (arp[i] == equalCheckSet.arp[i])
        {
          break;
        }
        else
        {
          continue;
        }
      }
    }
  }
  else
  {
    return false;
  }
  return true;
}

 

ltt.zip

[Sauron]

Does it print any of the lines you output in the main functions?

12 hours ago, duckypath said:

for (int i = 0; i < numElements; ++i)
    {
      for (int j = 0; j < equalCheckSet.numElements; ++j)
      {
        if (arp[i] == equalCheckSet.arp[i])
        {
          break;
        }

This is definitely wrong. For each value of i you're comparing arp to equalCheckSet.arp j times. this doesn't make sense - you probably don't want the second loop. Also, even then this doesn't do what you probably think it does; the condition in which you can break the loop and return an answer is that one of the values is not equal in both objects. So this should just be:

12 hours ago, duckypath said:

if (numElements != equalCheckSet.numElements)
  return false;
for (int i = 0; i < numElements; ++i)
{
  if (arp[i] != equalCheckSet.arp[i])
    return false;
}
return true;

 

 

[duckypath]

5 hours ago, Sauron said:

Does it print any of the lines you output in the main functions?

This is definitely wrong. For each value of i you're comparing arp to equalCheckSet.arp j times. this doesn't make sense - you probably don't want the second loop. Also, even then this doesn't do what you probably think it does; the condition in which you can break the loop and return an answer is that one of the values is not equal in both objects. So this should just be:

 

Depends, sometimes I get set 1 to print, other times like on repl.it, I can get it all to print, but the difference method doesn't work.

 

I think it comes down to the memory being all mismanaged, and giving me these odd results.

 

Somewhere I am accessing memory I am not supposed to according to the debug I ran.

[duckypath]

Just now, duckypath said:

Depends, sometimes I get set 1 to print, other times like on repl.it, I can get it all to print, but the difference method doesn't work.

 

I think it comes down to the memory being all mismanaged, and giving me these odd results.

 

Somewhere I am accessing memory I am not supposed to according to the debug I ran.

I would also get some double free() error messages, which was the cause to not finishing compiling before as well.

[duckypath]

5 hours ago, Sauron said:

Does it print any of the lines you output in the main functions?

This is definitely wrong. For each value of i you're comparing arp to equalCheckSet.arp j times. this doesn't make sense - you probably don't want the second loop. Also, even then this doesn't do what you probably think it does; the condition in which you can break the loop and return an answer is that one of the values is not equal in both objects. So this should just be:

 

https://onlinegdb.com/NAu1whXV0

 

This might also help, when you debug can see the segmentation errors.

[wanderingfool2]

On 11/14/2021 at 6:33 PM, duckypath said:

Set::Set(int arr[], int arrSize)

You haven't initialized numElements...and you are calling the Add function in that constructor.  So numElements could be anything, which could be the random issues you are experiencing.  Make sure to set all local variables nearby the top of the constructor if possible.

 

I know you said about the memory thing...but you really should add the delete [] arp in the extendset method.

 

Anyways, I think the not having numElements getting assigned is likely to be the main cause of the randomness (because depending what random number it got, it could work perfectly, crash while adding or crash while displaying)

[wanderingfool2]

On 11/15/2021 at 5:15 AM, duckypath said:

I would also get some double free() error messages, which was the cause to not finishing compiling before as well.

I have a bit more time to skim things over.  Not sure if it's practical or if you've come across the problems in the code.  But anyways, to clarify on my last post.  numElements is not set before being used in the add method; because it wasn't set in the constructor

Set::Set(int arr[], int arrSize)
{
  pSize = Set::DEFAULT_SIZE;
  arp = new int[pSize];
  numElements = 0; //This is whta you were missing.  From here it "works", in that you won't get the general random crashes
  for (int i = 0; i < arrSize; ++i)
  {
    add(arr[i]); // must add all the elements to arr.
  }   
}

To address the "double free()" message, well that's I believe because of your copy constructor and your deconstructor.  You are copying the pointers when you make a copy (instead of allocating new memory and copying the data held in the array).  This means when the object is destroyed it will delete the memory, but the other copied object still has the pointer and when that copied object is deleted it will try deleting the free memory.  That actually causes a situation as well where you are accessing memory that is already freed up (which is a bad thing).  An example of what I am referring to.

 

void memoryViolation() {
    int arr1[6] = {2, 1, 4, 3, 5, 4};
    Set set1(arr1,6);
    if(true) {
    	Set corruptingSet(set1); //At this stage, corrupting set is created and both point to the same memory
        set1.Add(6);
        corruptingSet.display(); //Will have the "6" element in it
        //End of if loop, so corruptingSet's deconstructor is called freeing the memory that set1 points to!!
    }
    set1.display();//This is a memory actually invalid
}

 

Then there is the memory leak here

void Set::extendSet()
{
  //dynamically creating new arr in memory equal to new size + old default.
    int *newArr = new int[pSize + Set::DEFAULT_SIZE];
    pSize += Set::DEFAULT_SIZE;
    for(int i = 0; i < numElements; i++)
    {
      newArr[i] = arp[i];
    }
    delete [] arp; //This is what you didn't add, since you need to make sure to free the memory
    arp = newArr;
}

 

[Unimportant]

Don't use naked new/delete. Read up about std::unique_ptr, then incorporate it into your code and all your errors (i.e. copy ctor does not deep copy, etc...) Will automatically become apparent if you carefully read the compilation errors.