Implementation of "Rule of Three" gone wrong

Question

Below is an erroneous implementation of "The rule of three", which I try to understand.

Debugging the program, I found that the debugger has a problem cleaning up int *k, which could be resolved either by defining int *k = nullptr or simply setting it to something reasonable in the copy constructor.

However, I do not understand how the resulting error of the program, an access violation, comes into existence.

I do know, after the copy assignment constructor v1's int *kno longer points to a valid memory address.

class Vector2 {
public:
    std::string name = "default";
    int* k;

    Vector2(int s, std::string n) : k(new int[s]), name(n) {
    }

    Vector2(const Vector2 &other)  {
        std::cout<< "Copy constructor: " << name << std::endl;
    }

    ~Vector2() {
        std::cout << "Deleting: " << name << std::endl;
        delete[] k;
    }

    void swap(Vector2& other) {
        using std::swap;
        swap(k, other.k);
    }

    Vector2& operator=(Vector2 other) {
        std::cout << "Copy assignment constructor: " << name << std::endl;
        swap(other);
        return *this;
    }
};


int main() {
        Vector2 v1 = Vector2(2, "v1");
        Vector2 v2 = Vector2(4, "v2");
        v1 = v2;
        std::cout << &v1 << " " << &v2 << std::endl;
        std::cout << &v1.k << " " << &v2.k << std::endl;
        return 0;
    }

Below is the console output of above's program:

Copy constructor: default
Copy assignment constructor: v1
Deleting: default
0000001B5611FA28 0000001B5611FA78
0000001B5611FA50 0000001B5611FAA0
Deleting: v2
Deleting: v1
16:18:42: The program has unexpectedly finished.

From C++11 and onward, it is the [Rule of five](https://cpppatterns.com/patterns/rule-of-five.html) though. — Neijwiert, Mar 13 '19 at 15:56
You also need to pass the Vector2 into your copy assignment operator by reference. Ideally as a const reference but I'm not sure if that's absolutely necessary — Tharwen, Mar 13 '19 at 16:14
@Tharwen that's the very point of the rule of three/five: you do **not** need to do that :) — sebrockm, Mar 13 '19 at 16:16
I would highlight that debugging programs is a skill that must be learnt - and using a debugger is an essential skill if you plan to do any programming (except for arduino work - where I don't think one exists). Learning how to do this is should be moved up your priority list rather than learning about the rule of 3/5 — UKMonkey, Mar 13 '19 at 16:24
You must have a *working*, non-buggy copy constructor, and a *working*, non-buggy destructor for the rule of 3 to work. Your copy constructor doesn't copy anything -- bug. Bottom line is that the copy constructor cannot be a "stub" function that is basically empty -- either you implement it fully, or declare it as `delete`-ed. You cannot run the program reliably with a stub copy constructor or actually any of the 3 functions as stubs to be filled in later. — PaulMcKenzie, Mar 13 '19 at 16:35

sebrockm · Answer 1 · 2019-03-14T11:02:45.580

It's actually very simple: Your copy constructor does not make a copy. In fact, it does not initialize any member, so any instance created by this constructor is filled with crap.

For the call of operator=(Vector2 other) the copy constructor is called to create other (this is the point of the rule of three), so other is filled with crap. Then you swap the valid k of this (aka v1) with the crappy k of other.

Then, when the destructor of v1 is called, it calls delete[] k on a crappy k --> access violation.

Solution

Make your copy constructor make a copy. Or at least, make it properly initialize k (e.g. to nullptr).

Zan Lynx · Answer 2 · 2019-03-13T16:03:12.743

0

Constructing other in operator= uses the copy constructor, which does not create a new copy of the pointed to value. Your copy constructor may not even copy k since it is a POD type and therefore not necessarily default constructed or default copied.

Then when it is destructed it tries to destroy it twice. Or depending on random factors like stack layout, it may not copy k at all and then it tries to delete an invalid pointer.

edited Mar 13 '19 at 16:03

answered Mar 13 '19 at 15:56

Zan Lynx

53,022
10
79
131

4

It is not destroyed twice. Newly constructed object does not initialize `k`, resulting in the crash when the destructor tries to delete this uninitialized value. – 1201ProgramAlarm Mar 13 '19 at 16:02
@1201ProgramAlarm Did you try it because I got a copy. OR the same pointer somehow. But you're technically right and I updated my answer. – Zan Lynx Mar 13 '19 at 16:03
@1201ProgramAlarm, I think so too - as written - k does not point to a valid memory address. At which step though does it become invalid? – Imago Mar 13 '19 at 16:04
@Imago It never *becomes* invalid. It was never valid to start with because it is never set explicitly by the copy constructor. – Zan Lynx Mar 13 '19 at 16:05
Why not? It had called the constructor for `v1`, so not using the copy assignment constructor makes above's code completely valid. – Imago Mar 13 '19 at 16:08
2

It isn't overwriting `v1` in `operator=`. It creates an entirely new copy because the function parameter is not a reference. – Zan Lynx Mar 13 '19 at 16:09
2

Or, OK. `v1` becomes invalid when `swap` overwrites its `k` with the `other.k` – Zan Lynx Mar 13 '19 at 16:10

score 0 · Answer 3 · answered Mar 13 '19 at 16:22

Your issue is in Vector2(const Vector2 &other)

You use this constructor in your operator = implicitly by passing by value; but you fail to assign k to any value in that constructor.

This results in swap replacing a valid k with an invalid k, and then deleting the invalid k; resulting in your crash.

Imago · Accepted Answer · 2020-01-10T17:06:56.900

The solution can be derived by laying out the exact sequence of events, e.g.: More print outs and testing, what parameters are called when:

Starting in: v1 = v2;

v2 calls copy constructor with the argument other (whatever other is), in particular: its int* k does not point to valid memory. For simplicity let's call this new Vector2 v3.
The copy assignment constructor is called now with v3.
Then we begin swap.

The error actually arises within the copy constructor, as v3 is not initialised properly in step 1 .

Step 2 and step 3 are basically "hiding", transferring the error from v3 onto v1.

The interesting question now is, how is v3 actually generated? Not by the default constructor!

Implementation of "Rule of Three" gone wrong

4 Answers4

Solution