Can't figure out where my memory management is going wrong.

[idknuttin]

I am practicing classes so I I tried to make a class about an animal, where the private attributes are the animals age, number of previous owners, and a sequence of strings that holds the names of the previous owners. I made the class and it compiles by itself, but when I try to set a previous owner I get an error for segmentation fault core dump, and when I try to call the copy assignment or copy constructor I get an error bad memory allocation. I tried to debug and figure out where I am going wrong with the memory but I can't figure it out.

These are just the errors I found so far and trying to fix, I am using the flag -std=c++14 when I compile in Ubuntu for nullptr.

#include<iostream>
#include<string>

class Animal{

private:
  int age;
  int numOwners;
  std::string* previousOwners;

public:
  Animal(const int & =0, const int & =0);
  Animal(const Animal &);
  void operator=(const Animal &);
  Animal(Animal &&);
  void operator=(Animal &&);
  ~Animal();
  void setAge(const int &);
  void setNumOwners(const int &);
  void setPreviousOwner(const int &, const std::string &);
  int getAge() const;
  int getNumOwners() const;
  std::string getPreviousOwner(const int &) const;
};

Animal::Animal(const int &a, const int &n){
  age = a;
  numOwners = n;
  if(numOwners > 0){
    std::string* previousOwners = new std::string[numOwners];
  }
  else{
    std::string* previousOwners = nullptr;
  }
}

Animal::Animal(const Animal & copyFrom){
  age = copyFrom.age;
  numOwners = copyFrom.numOwners;
  if(numOwners > 0){
    std::string* previousOwners = new std::string[numOwners];
    for(int i=0; i<numOwners; ++i){
      previousOwners[i] = copyFrom.previousOwners[i];
    }
  }
  else{
    std::string* previousOwners = nullptr;
  }
}

void Animal::operator=(const Animal & copyAssignFrom){
  age = copyAssignFrom.age;
  numOwners = copyAssignFrom.numOwners;
  if(previousOwners != nullptr){
    delete[] previousOwners;
  }
  if(numOwners > 0){
    std::string* previousOwners = new std::string[numOwners];
    for(int i=0; i<numOwners; ++i){
      previousOwners[i] = copyAssignFrom.previousOwners[i];
    }
  }
  else{
    std::string* previousOwners = nullptr;
  }
}

Animal::Animal(Animal && moveFrom){
  age = moveFrom.age;
  moveFrom.age = 0;
  numOwners = moveFrom.numOwners;
  moveFrom.numOwners = 0;
  previousOwners = moveFrom.previousOwners;
  moveFrom.previousOwners = nullptr;
}

void Animal::operator=(Animal && moveAssignFrom){
  age = moveAssignFrom.age;
  moveAssignFrom.age = 0;
  numOwners = moveAssignFrom.numOwners;
  moveAssignFrom.numOwners = 0;
  if(previousOwners != nullptr){
    delete[] previousOwners;
  }
  previousOwners = moveAssignFrom.previousOwners;
  moveAssignFrom.previousOwners = nullptr;
}

Animal::~Animal(){
  if(previousOwners != nullptr){
    delete[] previousOwners;
  }
}

void Animal::setAge(const int &a){
  age = a;
}

void Animal::setNumOwners(const int &n){
  if(n <= 0){
    if(previousOwners != nullptr){
      delete[] previousOwners;
      numOwners = 0;
    }
  }
  else{
    if(numOwners != n){
      int min = n;
      if(numOwners < n){
          min = numOwners;
      }
        std::string* newPrevOwners = new std::string[n];
        for(int i=0; i<min; ++i){
          newPrevOwners[i] = previousOwners[i];
        }
        if(previousOwners != nullptr){
          delete[] previousOwners;
        }
        previousOwners = newPrevOwners;
        numOwners = n;
    }
  }
}

void Animal::setPreviousOwner(const int &i, const std::string &name){
  previousOwners[i] = name;
}

int Animal::getAge() const{
  return age;
}

int Animal::getNumOwners() const{
  return numOwners;
}

std::string Animal::getPreviousOwner(const int &i) const{
  return previousOwners[i];
}

int main(){

  Animal first(5, 3);
  std::cout<<first.getAge() <<std::endl;
  std::cout<<first.getNumOwners() <<std::endl;
  first.setAge(6);
  std::cout<<first.getAge() <<std::endl;
  first.setPreviousOwner(0, "bob"); //error: segmentation fault core dumped

  Animal second(first); //error for bad memory allocation, this should call 
                        //copy constructor
  Animal third();
  third = first; //should call copy assignment, error:  cannot convert ‘Animal’ 
                 //to ‘Animal()’ in assignment

  return 0;
}

[Thomas1965]

In C++ you shouldn't use dynamic memory unless you really have to. You see the problems it can cause for beginners.
Better use a std::vector<Animal> std::vector<string>
https://www.codeguru.com/cpp/cpp/cpp_mfc/stl/article.php/c4027/C-Tutorial-A-Beginners-Guide-to-stdvector-Part-1.htm

[keskiverto]

The use of std::vector is use of dynamic memory. The difference is that if you do it yourself, then you have to know what you are doing, but with the vector somebody else has already done most of the hard work for you.


What is thaught to beginners is not "modern C++". It is "thinking". You are given very simple (Lego/Minecraft) building blocks and you have to build complex models. You are forced to think to get things right. Modern C++ assumes that you can already think and simply provides more elaborate blocks to make the "work" easier.

[idknuttin]

i Know how to use vectors, I am trying to learn how to use pointers in c++ so that I can understand how c++ manages memory, that's why I am trying to figure out how to make this work with pointers.

[Repeater]

Animal third();

This declares the existence of a function named third that returns an Animal and accepts no parameters. I bet that's not what you meant.

[Repeater]

In the constructor Animal::Animal(const int &a, const int &n) you never set the object's previousOwners variable.

You are creating a whole new variable of that name, and making it point to an array. Then, the constructor ends, that whole new variable is discarded (memory leak) and the object's internal previousOwners variable hasn't been touched, so it will be pointing into random memory.

[Repeater]

See also Animal::Animal(const Animal & copyFrom) and Animal::operator=(const Animal & copyAssignFrom) ; creating a whole new previousOwners pointer, doing things to it, and then discarding it, leaving the object's own internal previousOwners variable untouched.

[Repeater]

When you build, also use the flag -Wall and -Wextra and -Wshadow (and probably more too). -Wshadow would have warned you that you were shadowing those variables by creating new ones of the same name.

[idknuttin]

@Repeater, thank you, I forgot that I wasn't suppose to put std::string* before the previous owners because this means it was creating a new array and not using the array from private, I fixed all of the instances and it works now.