-1

I'm struggling with a small class I created. It is supposed to hold an array with data, that has to be deleted when the class object is deleted. Hence:

#include <stdio.h>
#include <stdlib.h>
#include <iomanip>
#include <iostream>
#include <fstream>
#include <math.h>    // for sin/cos
#include <algorithm> // for std::swap

class Grid
{
public:
    unsigned int dimX_inner, dimY_inner;
    double *x_vector1;
    unsigned int ghostzone;

    int dimX_total, dimY_total;

    double h_x, h_y;

    Grid(const unsigned int dimX_inner, const unsigned int dimY_inner, const unsigned int ghostzone = 0);
    ~Grid();
};

Grid::Grid(unsigned int gridsize_x, unsigned int gridsize_y, unsigned int ghostzoneWidth) : dimX_inner(gridsize_x),
                                                                                            dimY_inner(gridsize_y),
                                                                                            x_vector1(new double[(gridsize_x + 2 * ghostzoneWidth) * (gridsize_y + 2 * ghostzoneWidth)])

{
    ghostzone = ghostzoneWidth;
    dimX_total = gridsize_x + 2 * ghostzoneWidth;
    dimY_total = gridsize_y + 2 * ghostzoneWidth;
    h_x = (double)1.0 / (gridsize_x - 1);
    h_y = (double)1.0 / (gridsize_y - 1);
}
Grid::~Grid()
{
    delete[] x_vector1;
}

That's all I need for now. Added a little main:

int main()
{
    int problemsize = 5;

    Grid *grid1 = new Grid(problemsize, problemsize);

    delete[] grid1;

    return 0;
}

I've written a similar class before, that ran without problems. Granted most of it was taken from some online exercises/examples. But in this case I get a Segmentation fault when running the program (at the delete line). I guess it's the constructor, that somehow constitutes the problem. What I'd like to ask:

  1. What might be the cause of the segmentation fault?
  2. Is the constructor in the code "acceptable" (coding style wise)?
  3. Would I have to make any additional considerations when adding another data object to the class (e.g. an array *x_vector2)?

Short remark: I'm running this using WSL (don't know whether this is relevant)

luka b.
  • 31
  • 4
  • 4
    `delete[] grid1;` you used the wrong delete. new and delete match so do new[] and delete[] – drescherjm Apr 21 '22 at 12:42
  • Your class seems to miss [this](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – πάντα ῥεῖ Apr 21 '22 at 12:42
  • 1
    your code has more issues than you might be aware of. Read https://en.cppreference.com/w/cpp/language/rule_of_three. And `delete[]`ing what you created via `new` is UB. Use `std::vector` and say goodbye to raw new/delete – 463035818_is_not_an_ai Apr 21 '22 at 12:43
  • 2
    Also don't use `new Grid`. Use a stack allocated `Grid`. If you only use it for a single function and don't need it on the heap, don't put it there. It not only is prone to errors like this and memory leaks, but is considerably slower since `new` needs to get some heap memory so it might make a system call, but it at least has to go through some allocator, while stack based objects are generally a few lines of assembly for space allocation – Lala5th Apr 21 '22 at 12:43
  • 1
    `double *x_vector1;` --> `std::vector x_vector1;`. Once you do that, all of the issues pointed out go away, the calls to `new[]` and `delete[]` go away, the `~Grid()` destructor goes away or can be declared as `~Grid() = default;`, etc. – PaulMcKenzie Apr 21 '22 at 12:45
  • A bit about terminology: the data in the class needs to be deleted when the object is **destroyed**. An object can be destroyed because it was created with `new` and later deleted, but it can also be destroyed because it was created as a local variable and it went out of scope. The class doesn't care (generally) which way it was created; its destructor cleans everything up when an object gets destroyed. – Pete Becker Apr 21 '22 at 12:45
  • *What might be the cause of the segmentation fault?* -- With your current code: `int main() { Graph g1(5,5), Graph g2 = g1; }` -- Just that alone causes a [double-delete error](http://coliru.stacked-crooked.com/a/1049def53b62511d) due to the destructor being called twice on the same pointer value. However [this version](http://coliru.stacked-crooked.com/a/6c3237b7f92288d4) shows no such issues. – PaulMcKenzie Apr 21 '22 at 13:00

1 Answers1

2

What might be the cause of the segmentation fault?

You cannot delete[] what you created via new. new / new[] must be matched with their respective counter part delete / delete[]. Mixing them results in undefined behavior. Actually you should avoid any use of new or delete when possible. For example your main should look like this:

int main()
{
    int problemsize = 5;    
    Grid grid1{problemsize, problemsize};
}

The destructor of grid1 will be called when it goes out of scope.

Is the constructor in the code "acceptable" (coding style wise)?

No. You use the member initializer list for some members, but why not for all? For fundamental types the difference is negligible, but style-wise you should always prefer initialization rather than initialization plus assignment in the constructor body.

Would I have to make any additional considerations when adding another data object to the class (e.g. an array *x_vector2)?

You need not add another member. The class is already broken due to ignoring the rule of 3/5 (https://en.cppreference.com/w/cpp/language/rule_of_three). Instead of manually managing the memory you should delegate that to containers and/or smart pointers. In your case a std::vector seems to fit. The destructor can then be ~Grid() = default; (cf. rule of zero).

Managing a resource is already enough for a class to do. Consequently a class that does something should not in addition manage memory. In extremely rare cases you need to write a custom type that manages memory, but most of the time a standard container or a smart pointer should suffice.

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185