0

I wrote an example which leads with Visual to a crash. I think the critical section is here:

A a;
B b(a);
pair p(a, b);
res.push_back(p);

I assign a temporary which takes a reference to another temporary and move it into a vector. I also noticed that the program seems not to crash with GCC. I think the problem is, that the reference is invalidated since the moment I move the objects into the vector. Do I have to use new?

// ConsoleApplication1.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"


// Example program
#include <iostream>
#include <string>
#include <vector>
#include <math.h>

class Materials {
public:
  Materials() = default;
};

class A {
private:
  std::vector<Materials> _storage;

public:
  void setStorage(const std::vector<Materials> &val) {
    _storage = val;
  }
};

class B {
private:
  A &_a;

public:
  B(A &foo) : _a(foo) {}

  void run() {
    std::vector<Materials> val;
    for (int i = 0; i < 1000; i++) {
      val.push_back(Materials());
    }

    _a.setStorage(val);
  }
};

struct pair {
  pair(const A &a, const B &b)
    : _a(a)
    , _b(b)
  {
  }

  A _a;
  B _b;
};

std::vector<pair> make_list() {
  std::vector<pair> res;
  for (int i = 0; i < 10; i++) {
    A a;
    B b(a);
    pair p(a, b);
    res.push_back(p);
  }
  return res;
}

int main()
{
  std::vector<pair> list = make_list();
  B &ref = list[1]._b;
  ref.run();
}
dgrat
  • 2,214
  • 4
  • 24
  • 46
  • 1
    Did you debug to find the statement which is resulting in the crash? – CinCout Jul 04 '17 at 11:36
  • 2
    Why don't you just copy or move into the object, instead of storing a doomed reference? I think this is an X/Y question. What are you trying to do, and why did you think that using a reference would be a good idea, especially to a temporary? Also, I don't know why you reinvent `pair`. – underscore_d Jul 04 '17 at 11:36
  • 1
    You are right I should make the minimal a little easier. – dgrat Jul 04 '17 at 11:42
  • @dgrat Yes please. It's easy to miss that loop otherwise :P – CinCout Jul 04 '17 at 11:45
  • 2
    @CinCout: Yes, they did, and their analysis is correct. Now to find a solution (which the debugger cannot do). Ultimately the solution is a redesign, so underscore_d's follow-up questions are crucial. In short there are a lot of copies here that aren't needed. – Lightness Races in Orbit Jul 04 '17 at 11:51

2 Answers2

0
#include <iostream>
#include <string>
#include <vector>
#include <math.h>

class Materials {
public:
    Materials() = default;
};

class A {
private:
    std::vector<Materials> _storage;

public:
    void setStorage(const std::vector<Materials> &val) {
        _storage = val;
    }
};

class B {
private:
    A _a;

public:
    B(A &foo) : _a(foo) {}

    void run() {
        std::vector<Materials> val;
        for (int i = 0; i < 1000; i++) {
            val.push_back(Materials{});
        }

        _a.setStorage(val);
    }
};

struct pair1 {
    pair1(const A &a, const B &b)
        : _a(a)
        , _b(b)
    {
    }
    //pair1(const pair1& p) :pair1(p._a, p._b) {}

    A _a;
    B _b;
};

std::vector<pair1> make_list() {
    std::vector<pair1> res;
    for (int i = 0; i < 10; i++) {
        A a;
        B b(a);
        pair1 p(a, b);
        res.push_back(p);
    }
    return res;
}

int main()
{
    std::vector<pair1> list = make_list();
    B &ref = list.at(1)._b;
    ref.run();
}

1. You don't use move sematics

  1. You can change Materials() because look like a function Materials{} look like a constructor

  2. Problem is A &_a change to A _a

  3. Probably name struct, because in standard library is pair, and you define too pair
  4. You should use list.at(1) no list[1], because first solution give more information ( exception etc.)
21koizyd
  • 1,843
  • 12
  • 25
  • 2 is just plain wrong; those are just two different ways of invoking a constructor. Also, disagreed on 5: `operator[]` is fine and less overhead if there is no expectation that an invalid index can be given (e.g. from arbitrary user input). And what does "Section critic is" mean, maybe "The most important thing to change is"? – underscore_d Jul 04 '17 at 12:12
  • with constructor i though about something different, so i improved, 5. is more safety during writing code. – 21koizyd Jul 04 '17 at 12:17
  • I see your point about 5. Personally, I would prefer to use a library with debug-mode options to check indices in `operator[]`, then turn that off for the release build - which avoids either putting the slow `.at()` into released code in cases where it is never needed, or the user having to do a lot of (potentially error-prone) find-and-replace. – underscore_d Jul 04 '17 at 12:21
-3

Instances of A and B classes created in function make_list are local and valid only within the functions. Objects are allocated in stack and content may be destroyed when program leaves make_list. "new" could help because objects are allocated in heap and will be there until allocated memory will be freed.

Smike
  • 1