2

I have a class method that works with a copy of an object (*this, to be exact). The leaks occur within the overloaded assignment operator - that's what Visual Leak Detector says, anyway. What I'm doing is working with copy and if the work done is satisfactory I copy that newly created object back. I've also implemented a custom destructor, copy constructor and assignment operator because the problem occurs with dynamically allocated memory, obviously. My experience with C++ is quite limited so there could be some evil stuff in the code.

I will provide more info if needed.

Problematic method:

bool Grid::SurroundShipSquares(int top, int bottom, int left, int right)
{
    // copying itself
    Grid gridCopy(*this);
    Square** squaresCopy = gridCopy.GetSquares();
    for (int i = top; i <= bottom; ++i)
    {
        for (int j = left; j <= right; ++j)
        {
            if (squaresCopy[i][j].GetState() != SquareState::Vacant)
                return false;
            (squaresCopy[i][j]).SetState(SquareState::Unoccupiable);
        }
    }
    // the problem occurs here
    *this = gridCopy;
    return true;
}

Copy constructor:

Grid::Grid(const Grid& source)
{
    _position = source._position;
    _size = source._size;
    int dimensions = static_cast<int>(_size);
    _squares = new Square*[dimensions];
    for (int i = 0; i < dimensions; ++i)
    {
        _squares[i] = new Square[dimensions];
        for (int j = 0; j < dimensions; ++j)
        {
            _squares[i][j] = source._squares[i][j];
        }
    }
}

Assignment operator:

Grid& Grid::operator=(const Grid& source)
{
    if (this == &source) 
        return *this;
    _position = source._position;
    _size = source._size;
    int dimensions = static_cast<int>(_size);
    _squares = new Square*[dimensions];
    for (int i = 0; i < dimensions; ++i)
    {
        _squares[i] = new Square[dimensions];
        for (int j = 0; j < dimensions; ++j)
        {
            _squares[i][j] = source._squares[i][j];
        }
    }
    return *this;
}

Destructor:

Grid::~Grid()
{
    int dimensions = static_cast<int>(_size);
    for (int i = 0; i < dimensions; ++i)
    {
        delete[] _squares[i];
    }
    delete[] _squares;
}
Venom
  • 1,107
  • 4
  • 21
  • 46
  • 5
    Save yourself all the trouble and use the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) instead. – Daniel Frey Aug 22 '14 at 21:35
  • Unfortunately, this question has turned into yet another demonstration of why my participation at this part of the SE network has dropped off so lately. – David Hammen Aug 22 '14 at 22:59
  • @DavidHammen I am sorry it came to this, but surely I'm not the one to blame. All I did was post an honest, newbie question. – Venom Aug 22 '14 at 23:04
  • 2
    @Venom: You most surely are not the one to blame. On the one hand, you should think seriously of using the tools in the standard library. On the other hand, you will need to eventually learn to write safe code and properly manage resources. There's a crowd of serial downvoters at this site that won't let others answer the latter part of your question. Do what Daniel Frey said in his comment. Learn the copy-and-swap idiom. And you should think of using std::vector **if it suits your needs**. – David Hammen Aug 22 '14 at 23:09
  • 2
    @DavidHammen You are absolutely free to answer the question the way you did. Other people consider it harmful though, and they explained to you (in the comment section) why. It's their choice. Stop the whole whining victimization. – Shoe Aug 22 '14 at 23:11
  • I'd also correct the last statement to "Always use `std::vector` unless there's a good reason not to; and if you really have to use `new` and `delete`, at least use a *smart pointer*". – Shoe Aug 22 '14 at 23:15
  • @DavidHammen I will have to learn how to allocate & deallocate memory eventually (it is an important feature of C and C++) but I simply haven't got the time right now, this school project (battleship clone) I'm working on has to be finished in 10 days. Luckily, the design part is decent, implementation is always tricky. I've also been working with languages that have garbage-collectors (C#, Java, etc.) so that quickly "spoiled" me, I guess. – Venom Aug 22 '14 at 23:24
  • School assignments ALMOST ALWAYS never let the student use anything in the `STL`.. aka `std::vector`, `std::string`, etc.. It's probably why you've been using `new` in the first place. Also, be careful about using vectors with matrices and games.. seeing as this is a battleship game. Especially if you value performance. `std::vector` will always initialise the underlying memory to default/0 before allowing you to write to it. Might not always be desirable. But again, if you aren't allowed to use `std::vector`, all the other answers were deleted and you'd now be screwed by StackOverflow logic. – Brandon Aug 22 '14 at 23:27
  • @Brandon True, they usually don't but this is an exception because it's not really a "Hello, World!" kind of app. Also, it has to be a GUI app, not a console one. What I'm doing now is writing a back end that will be used in a Qt project. I sincerely hope that Qt add-in for VS is decent. – Venom Aug 22 '14 at 23:30
  • School assignments almost NEVER need to be performant to the point of avoiding of default initialization. If his school is one of the few that let him use STL then you are screwing with him now. – Shoe Aug 22 '14 at 23:30
  • No. I'm not screwing with him. I'm making sure he has permission to use it for school, which no one else seemed to address at ALL. I know that it doesn't need performance for "school" which is why I just said "be careful" and not "don't use it.. it'll kill you". I mentioned "one reason" not to just in case. No where did I convince or attempt to convince OP to ignore STL. Don't be so jumpy. I am also a student.. but my assignments don't let me use it :l – Brandon Aug 22 '14 at 23:32
  • If he has a moronic teacher like that, it's his responsibility to mention it, not our responsibility to psychic it. – Puppy Aug 23 '14 at 08:38
  • I would have mentioned it were that the case, but luckily it is not. – Venom Aug 23 '14 at 14:48

1 Answers1

5

The problem with your code is that you manage all your resources manually. This is terribly unsafe and a massive headache to do correctly, as is aptly demonstrated by both the existing answers being wrong.

Use std::vector. This class will automatically manage all the memory for you, freeing you from having to do it yourself. This will greatly simplify your code, as well as making it correct.

Also, self-assignment checking is an ancient anti-pattern. Do not include a check for self-assignment. If your assignment operator (you shouldn't really need to write your own with std::vector-based memory management in most cases) cannot handle self-assignment without a special case, it is broken.

Puppy
  • 144,682
  • 38
  • 256
  • 465
  • Could I use a valarray for storing 2D matrices? I am asking because it will positively stay withing fixed dimensions. Like this, for example - std::valarray()> _squares. – Venom Aug 22 '14 at 21:56
  • You could do, but consider boost::multi_array instead. Your existing code is equivalent to a vector of vectors anyway. std::valarray is a bit of an orphaned child that nobody likes or uses. Ultimately, the exact choice of container is up to you, depends on the exact semantics you want to express, and less important here than using one in the first place. Typically, programmers just use vector unless they have a particular need to use something else. – Puppy Aug 22 '14 at 21:58
  • I notice that multi_array's size still needs to be known at compile time and that unfortunately doesn't work in my case. In my previous comment I meant that the containers size, once initially determined, stays fixed throughout its lifetime. C++14 proposes a dynarray, but there are obviously no implementations available yet. – Venom Aug 22 '14 at 22:05
  • @Venom No, `boost::multi_array`'s number of *dimensions* needs to be known at compile time. The sizes can be specified at runtime using `boost::extents`. Look at some of the examples in the documentation. – Praetorian Aug 22 '14 at 22:07
  • @Venom: The *extent* (i.e. number of dimensions) is fixed at compile-time. Your code also has a fixed number of dimensions at compile-time (2). The size, i.e. number of elements, is not fixed until run-time. C++14's dynarray is absymal crap. – Puppy Aug 22 '14 at 22:07
  • 1
    Just use `std::vector` for dynamically sized arrays and `std::array` for statically sized arrays. It's *that* simple. – Shoe Aug 22 '14 at 22:09
  • Yes, you certainly don't need anything more complex than that the vast, vast majority of the time. – Puppy Aug 22 '14 at 22:10
  • This is such a bad answer. Eigen provides much better solutions than a vector of vectors. My experience is that for a matrix, a vector of vectors is the wrong answer the vast, vast, vast majority of the time. – David Hammen Aug 22 '14 at 22:18
  • Puppy, could you expand on the not-C++14 `std::dynarray` being crap? Because I don't quite see that yet. – Deduplicator Aug 22 '14 at 22:19
  • @Deduplicator: It's randomly not actually the thing you want, you can't tell if it is or not, if it is then you get a bunch of O(N) moves and random UB, and there's no reason to even want that anyway because more efficient heap allocators can match the performance anyway. – Puppy Aug 22 '14 at 22:40
  • @DavidHammen: You whine about performance. I see zero indication that the OP's code is performance sensitive, and therefore zero rationale to justify bringing in an external dependency. – Puppy Aug 22 '14 at 22:41
  • I am not whining about performance. I typically don't give two hoots about performance during design or initial implementation. I do care about a match of functionality, and a vector of a vector is usually an incredible mismatch, not offering capabilities I do need and offering capabilities I do not need and in fact do not want present. For example, the ability to change the size of what is supposed to be a fixed size array is a big broke. – David Hammen Aug 22 '14 at 22:54
  • Yes, it's so terrible to re-use existing functionality just because it offers more than you need. It's so much effort to write a simple wrapper class that doesn't offer that implemented by it. It'd be so much simpler to just *re-roll the whole thing*. – Puppy Aug 22 '14 at 22:57
  • @Puppy Oh, man this multi_array is tricky... Simple assignment won't work, have to resize() first, every time. Not to mention dozens of compiling errors :) – Venom Aug 23 '14 at 12:50
  • @Puppy Yeah, I think I've got it to work now, I believe. Thank you for your advice. Only copies (newly created objects) need to be resized, references work fine with simple assignment. So it seems to me. – Venom Aug 23 '14 at 15:48
  • @Puppy I'm sorry for bothering you, but should I be worried about Level 4 compiler warnings (specifically, C4510 & C4610) concerning the multi_array? – Venom Aug 23 '14 at 16:17
  • 1
    @Venom: No. MSVC issues a whole bunch of totally spurious warnings. If the warning is emitted from in the Boost code, then ignore it- the Boost dev will already have looked for it. Basically nobody uses /W4 because the compiler emits so many useless warnings. – Puppy Aug 23 '14 at 21:08