0

I save objects in a list after creating them, but get a Segmentation fault when I try to read it from this list. When I print the pointers it shows me the same adress as the adress when creating then, so I dont know why I get a Seg.Fault.

Maybe you have a idea whats my error. Ty:

main:

int main()
{
  Computer computer("My Computer",1000);
  computer.addHardware("Monitor",250);
  computer.removeHardware("Monitor",250);
  computer.addHardware("Monitor",150);
  return 0;
}

Computer.c:

#include "Computer.h"
Computer::Computer(std::string name, int budget) : name_(name), budget_(budget),momentary_cost_(0){}

Computer::~Computer(){}

void Computer::addHardware(const char* name, int cost)
{
  Hardware newHardware =  Hardware(name,cost);
  if(momentary_cost_ + cost < budget_)
  {
    HardwareList.push_back(&newHardware);
    momentary_cost_ += cost;
  }
  else
  {
    delete(&newHardware);
  }

}


void Computer::removeHardware(const char* name, int cost)
{
  for (auto it = HardwareList.begin(); it != HardwareList.end(); ++it)
  {

    if (std::string(name) == std::string((*it)->getName()) || cost == (*it)->getCost())
    {
      HardwareList.erase(it);
    //  HardwareList = (Hardware*)realloc(HardwareList, sizeof(HardwareList)*(HardwareList.size()-1));
      delete(&it);
    }
  }
}
zwerg4
  • 320
  • 3
  • 5
  • 12
  • 2
    `HardwareList.push_back(&newHardware);` - start with that. You're pushing the address of an automatic variable that will no longer exist once `addHardware` returns to it's caller. That leaves a *dangling* pointer that, when dereferenced, will invoke *undefined behavior*. And nowhere in this code should there be a `delete` if there is no corresponding `new`. And `removeHardware` is as-wrong, if not more so, than `addHardware`. You may need to review how dynamic allocation (which you don't need to manually do here anyway) works. – WhozCraig Oct 12 '18 at 09:10
  • 1
    You cannot `delete` what was not allocated on heap previously using `new`. No `delete`s are needed in your program. I'd recommend you to see our [list of good C++ books](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) instead of guessing the syntax :) – Yksisarvinen Oct 12 '18 at 09:10
  • you are right, now the Object should be ok: `void Computer::addHardware(const char* name, int cost) { Hardware *newHardware = new Hardware(name,cost); if(momentary_cost_ + cost < budget_) { HardwareList.push_back(newHardware); momentary_cost_ += cost; } else { delete(&newHardware); } }` – zwerg4 Oct 12 '18 at 09:16
  • While `delete &it` compiles, you want `delete *it`. "Insert `&` and `*` until it compiles" is not a robust development method. – molbdnilo Oct 12 '18 at 09:16
  • Useful rule of thumb: `delete` and `&` should not be seen together. – molbdnilo Oct 12 '18 at 09:19
  • 1
    @JeJo there's enough wrong in this code that anything short of a near-full-rewrite would be just enough information to be dangerous. The best advice on this page right now is the list of books Yksisarvinen linked. That's far better than cooking spaghetti code (throwing it at the wall different ways until something sticks). – WhozCraig Oct 12 '18 at 09:22

0 Answers0