0

I have a class:

class Land{
public:
  ~Land() { if(map != nullptr) Clean(); }

  bool Create();
  void PrintMap() const;
  void Clean();
private:
  int rows = 0, columns = 0;
  int **map = nullptr;

  bool CorrectDimensions() const;
  void CreateMap();
  bool FillMap();
};
bool Land::Create() {
  //
  printf("Matrix size:\n");
  if(scanf("%d %d", &(this->columns), &(this->rows) ) != 2 || !CorrectDimensions() )
    return false;

  CreateMap();
  printf("Values:\n");
  return FillMap();
}
void Land::CreateMap() {
  //
  this->map = new int*[rows];

  for(int column = 0; column < this->rows; column++)
    map[column] = new int[this->columns];
}
bool Land::FillMap() {
  //
  for(int row = 0; row < this->rows; row++) {
    for(int column = 0; column < this->columns; column++) {
      if( ! scanf("%d", &(this->map[row][column]) ) ) {
        return false;
      }
    }
  }

  return true;
}
void Land::Clean() {
  //
  for(int row = 0; row < this->rows; row++)
  delete [] this->map[row];
  delete [] this->map;
}

And my driver:

int main() {
  //
  Land l;

  if( ! l.Create() ) {
    IncorrectInput();
    return 1;
  }

  l.PrintMap(); //prints correct output
}

How I imagine my program should work:

  1. Default constructor gets called, no need for a custom one.
  2. Check if the input satisfies the requirements. If not, return false thus finishing the program. Memory hasn't been dynamically allocated yet (stays at nullptr), no problem.
  3. If yes, create a 2D array using calloc (I know I'm mixing C with C++, wanted to use class and vector is not available).
  4. Fill that 2D array with scanned values. If scanned values aren't ints, return false, thus finishing the program.
  5. Print the value of that array. Const function.
  6. Finish the program so destructor is called. Since I don't know if Create() ended prematurely (before calloc) or not, I default initialize int **map to nullptr. If I allocate, map won't be nullptr anymore and will need to be freed. Otherwise, destructor should free, that's what Clean() does.
  7. Clean() I followed the practices of dynamic allocation and freeing. calloc and free follow their reversed patterns. Debugging confirmed that how many callocs were called, that many frees were called.

With all of this, valgrind still reports errors (not reachable, actual errors). Specifically: total heap usage: 184 allocs, 23 frees. What caused this error, what have I missed?

EDIT: Initialized rows and columns members. Changed calloc() with C++ new and delete.

Sorry
  • 532
  • 2
  • 6
  • 19
  • `calloc` and `free` in C++? – John3136 Nov 24 '17 at 02:15
  • @John3136 see `(I know I'm mixing C with C++, wanted to use class and vector is not available).` – Sorry Nov 24 '17 at 02:16
  • 3
    Doesn't stop you using `new` and `delete` – John3136 Nov 24 '17 at 02:17
  • Your program will leak memory in a whole variety of ways, even if you "fix" the problem. What if in the middle of the creation of your matrix, one of the calls to `calloc` fails? How are you going to recover that memory? – PaulMcKenzie Nov 24 '17 at 02:19
  • @John3136 Reworked using `new` and `delete`. Valgrind reports identical error. – Sorry Nov 24 '17 at 02:27
  • 1
    Also, something as simple as `{Land l; }` invokes undefined behavior due to the member variables not being initialized in the `Land` object, and the destructor will be called using an uninitialized `rows` and `map` values. – PaulMcKenzie Nov 24 '17 at 02:27
  • @PaulMcKenzie Yes, I should've checked after every calloc. Still, this isn't the problem of this case or is it? – Sorry Nov 24 '17 at 02:28
  • Interesting fun fact: What you are creating is an array of arrays, not a 2D array. For an interesting take on how to make a 2D array give the following FAQ entry on making a matrix a read: https://isocpp.org/wiki/faq/operator-overloading#matrix-subscript-op – user4581301 Nov 24 '17 at 02:28
  • What is this `IncorrectInput`? Does it trigger an exception or terminate the program? – user4581301 Nov 24 '17 at 02:30
  • @LeNguyenDuyAnh -- I suggest getting rid of the input routines that return prematurely if someone enters a wrong value. Make sure the basic things do not leak first. Right now, there are fundamental problems with your design, as pointed out by my comment. – PaulMcKenzie Nov 24 '17 at 02:31
  • @user4581301Just prints error message. – Sorry Nov 24 '17 at 02:32
  • Unable to reproduce. (Anyone know an online valgrind?) Might be something silly happening in the code that hasn't been provided. Unrelated: `this->map = (int**)calloc( (size_t)this->rows, sizeof(int*) );` `calloc` is pointless here. You are about to overwrite all of the pointers in the next loop. – user4581301 Nov 24 '17 at 02:41
  • @user4581301 Just tried using valgrind in [**CS50's IDE**](http://cs50.io) and there valgrind showed no memory leaks. So is it possible my valgrind (MacOS Sierra) is causing problems? – Sorry Nov 24 '17 at 03:18
  • @LeNguyenDuyAnh -- Your updated code will cause a memory leak if `new` throws an exception in the middle of that loop. If you want to duplicate the issue, give a very large number to `rows` and `columns`. So no matter how you changed your code, you're right back at the situation where leaks will occur. If you insist on doing things this way, then [this way of allocation](https://stackoverflow.com/questions/21943621/how-to-create-a-contiguous-2d-array-in-c/21944048#21944048) gives you a fighting chance to rollback the previous allocations if `new` throws, as there are only 2 allocations. – PaulMcKenzie Nov 24 '17 at 06:03

1 Answers1

0

There is no memory-leak. calloc doesn't throw exception.

BTW, Null-pointer deference is detected in Land::CreateMap and meny other places. Don't forget checking returned value of calloc

updated

For example,

class foo {
  int* p1;
  int* p2;
  foo()
  {
    p1 = new int;
    p2 = new int;
  }
  ~foo()
  {
    delete p1;
    delete p2;
  }
};
int main()
{
  foo val;
}

In Ctor of foo, when p2 = new int; throw exception, Dtor of foo will not called, so, Anyone can delete p1!

So, when you want to allocate memory with non-noexcept operator new in ctor, You must use RAII wrapper. C++11 or later has RAII wapper class to allocate memory, that is std::unique_ptr or std::shared_ptr.

BTW,

vector is not available

What's the environment are you using?

yumetodo
  • 1,147
  • 7
  • 19