C++ why is my code not blowing up/segfaulting?

Question

I have the following code:


vector <Rect> detectAndDisplay(Mat frame) {
    Mat frame_gray;
    vector <Rect> detected;


    //load the cascade
    if (!cascade.load(CASCADE_NAME)) {
        printf("--(!)Error loading Cascade\n");
        std::exit(0);
    };


    // 2. Perform Viola-Jones Object Detection
    cascade.detectMultiScale(frame_gray, detected, 1.1, 1, 0 | CV_HAAR_SCALE_IMAGE, Size(50, 50), Size(500, 500));


    // 2.5 Merge overlapping rectangles
    groupRectangles(detected, 1, 0.8);
    std::vector <Rect> singles;
    std::vector <Rect> overlaps;
    for (auto rect_1: detected) {
        bool isSingle = false;
        for (auto rect_2: detected) {
            if (rect_1 == rect_2) {
                continue;
            }
            bool intersects = ((rect_1 & rect_2).area() > 0);
            if (intersects) {
                isSingle = false;
                break;
            }
        }
        if (isSingle) {
            singles.push_back(std::move(rect_1)); //MOVING 
        } else {
            overlaps.push_back(std::move(rect_1));
        }
    }

    // 4. Draw box around faces found
    for (int i = 0; i < detected.size(); i++) {
        rectangle(frame, Point(detected[i].x, detected[i].y),
                  Point(detected[i].x + detected[i].width, detected[i].y + detected[i].height),
                  Scalar(0, 255, 0), 2);
    }
    return detected;
}

When I compile and run, step 4 works fine. My question is, why? I have moved its content into two different vectors. As such I was expecting to hit a segfault or something.

What does move assign/construct for `Rect` look like? Unless they specifically do something to make the original `Rect` invalid, it would likely still work without issue. — ChrisMM, Nov 26 '19 at 15:36
"_As such I was expecting to hit a segfault or something._" Why would you **expect** segfault? Segfault can only come as one of possible behaviors of undefined behavior. And undefined behavior is undefined. It can manifest itself as segfault, or, as working as expected, at current execution. — Algirdas Preidžius, Nov 26 '19 at 15:38
In your class, add a `bool dead = false;` member variable. Set it to `true` when moved-from, set it to `false` when assigned-to. In other functions, at the beginning add `assert(!dead);`. — Eljay, Nov 26 '19 at 15:41

Evg · Accepted Answer · 2019-11-26T17:10:31.373

4

I have moved its content into two different vectors.

You haven't. detected is not modified in the loop, no matter whether Rect has a move constructor or not.

First, in for (auto rect_1: detected) you make a copy of a vector element (auto is not a reference), and then in std::move(rect_1) you move from that copy.

To move from detected, rect_1 should be a reference: for (auto& rect_1: detected).

Let's assume that you've fixed the for loop and detected elements are moved from.

Suppose that Rect looks like this:
```
struct Rect {
    int x;    
    Rect(Rect&& other) : x(other.x) { }
};
```
This move-constructor will not modify other, so .push_back(std::move(rect_1)) will not modify rect_1 and detected.
Suppose we define the move constructor in a different way:
```
struct Rect {
    int x;
    Rect(Rect&& other) : x(other.x) {
        other.x = 0;
    }
};
```
Now it will modify other by setting other.x to zero. After the loop, all Rects in detected will have x = 0. If your program relies on the condition x != 0, it might crash.
If you do not declare a move constructor at all, Rect will have a trivial implicitly-declared move constructor that will just make a copy of Rect. So, from the point of view of move construction
```
struct Rect {
    int x;
};
```
will be equivalent to that in 1. In this case detected will not be modified either even after all its elements have been moved from.

Simple demo

edited Nov 26 '19 at 17:10

answered Nov 26 '19 at 15:55

Evg

25,259
5
41
83

Great answer, thanks. I'm just a beginner, but why must movable objects be references to the values? Why aren't the values movable themselves? – nz_21 Nov 26 '19 at 19:12
1

Consider simple example: `int num = 1; auto x = num; x = 0;`. What is `num`? Still `1`. If you want to move from `detected[0]`, you should move from `detected[0]`, not from a copy. In your code you first make a copy: `auto x = detected[0]`, and then move from that copy: `push_back(std::move(x))`. When you move from `x`, you modify `x`, but modifying a copy cannot modify the original object `detected[0]`. In contrast, if you write `auto& x = detected[0]`, then `x` is a reference, and modification of `x` is modification of `detected[0]`. `int num = 1; auto& x = num; x = 0;`. What is `num`? `0`. – Evg Nov 26 '19 at 19:23
Evg ah I see. Am I right in thinking that this will have the desired move effect? `for (int i=0;i – nz_21 Nov 26 '19 at 19:29
@nz_21, this will move each element into local `x`, which will be destroyed when it goes out of scope (on each iteration). In case 2 this will leave `detected` will all zeros. In cases 1 and 3, `detected` will not be modified. In a more realistic case of `std::vector>` (`unique_ptr` has a non-trivial move constructor) this loop will destroy all objects managed by these unique pointers, leaving the vector with null pointers. – Evg Nov 26 '19 at 19:34

score 2 · Answer 2 · answered Nov 26 '19 at 15:54

Perhaps the biggest danger to Undefined Behaviour is not that it'll blow up in your face, but that it very, very often won't, and appears to be working fine until you change something else, such as running environment or hardware, the good old "it works on my machine" scenario; another UB area; or even nothing at all, such as a race condition.

Undefined Behaviour is not lazy terminology for a catastrophic error like segmentation fault or stackoverflow, nor a coy way of avoiding documentation for develop-specific errors. It may sometimes be a cop-out to examining a weird and technically incorrect code path, but ultimately undefined most often means inconsistent.

C++ why is my code not blowing up/segfaulting?

2 Answers2