7

This code will explode, right? As soon as the loop exits, the original instances will die with all their inner members so if they weren't PODs, any method like do_stuff which requires access to members of B will throw a segmentation fault, correct?

void foo() {
  std::vector<B> bar;
  for (int i = 0; i < 7; i++)
    bar.push_back(B(i, i, i));
  bar[3].do_stuff();
}

So, is there any way to do this without using a pointer? Or do you have to do this:

void foo() {
  std::vector<B*> bar;
  for (int i = 0; i < 7; i++)
    bar.push_back(new B(i, i, i));
  bar[3]->do_stuff();
  for (int i = 0; i < 7; i++)
    delete bar[i];
}
Ziezi
  • 6,375
  • 3
  • 39
  • 49
AturSams
  • 7,568
  • 18
  • 64
  • 98
  • 1
    The first way is fine. If you want Java, use `std::reference_wrapper`. – chris Aug 30 '15 at 17:11
  • also fwiw I don't think "POD" has anything to do with it, its just about life time of the objects. It's not legal to call methods on an object whose lifetime is expired, POD or not – Chris Beck Aug 30 '15 at 17:12
  • So weird.. I had the first version, kept getting seg fault => checked with Valgrind, it said it was B::do_stuff() => changed to the second version and now it works fine. Also thought the first version should work. So strage. – AturSams Aug 30 '15 at 17:14
  • 3
    Sounds like your class `B` is broken. – Benjamin Lindley Aug 30 '15 at 17:16
  • @BenjaminLindley, possible but why would using new B(i..) fix the breakage? Either way, will attempt to get a concise code example that fails (and one that works) if this happens again. – AturSams Aug 30 '15 at 17:16
  • Because then it no longer uses your class' broken value semantics. Most likely, your code is violating the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). – Benjamin Lindley Aug 30 '15 at 17:18
  • @BenjaminLindley please elaborate on this with an example on how you can break the code like that? Would be very informative and a really good answer. – AturSams Aug 30 '15 at 17:19
  • Check the link in my previous comment. – Benjamin Lindley Aug 30 '15 at 17:23
  • Even if you had to use the second version for some reason, you should use `std::unique_ptr` (to model the ownership) instead of raw `new`/`delete`. – Emil Laine Aug 30 '15 at 18:06
  • Post your `B` class and we can probably show you how it's broken. – Sebastian Redl Aug 30 '15 at 18:33
  • @SebastianRedl I'd like to do that but it'll break the question (make it unnecessarily convoluted). – AturSams Aug 31 '15 at 06:02
  • @BenjaminLindley - Is it possible, that because class B has a member that doesn't have a default constructor, the copy of an instance is faulty? – AturSams Aug 31 '15 at 06:13

4 Answers4

12

The first code example is valid.

std::vector will make a copy of the objects you pass them with push_back (or will move them in place with C++11 if you're pushing a temporary) and it will keep all of the instances alive as long as the vector itself is alive.

The destruction will happen when you exit the function, not when you exit the loop.

6502
  • 112,025
  • 15
  • 165
  • 265
7

The first code is better than the second one.

The B instances will be movedsince C++11/copiedpre-C++11 into the vector, so they will not fall out of scope after the loop — only after the vector falls out of scope.

If you want to get the absolutely optimal performance, then do this:

void foo() {
  std::vector<B> bar;
  bar.reserve(7);
  for (int i = 0; i < 7; i++)
    bar.emplace_back(i, i, i);
  bar[3].do_stuff();
}

This will guarantee only one reallocation, and the elements are constructed directly inside the vector (instead of moving or copying them there) as per Marc Glisse's comments.

Community
  • 1
  • 1
Emil Laine
  • 41,598
  • 9
  • 101
  • 157
  • 3
    If it was C++11, it should use emplace_back and avoid moving at all. – Marc Glisse Aug 30 '15 at 18:40
  • 1
    @MarcGlisse Is moving bad / dangerous? – AturSams Aug 31 '15 at 06:01
  • @zehelvion No, it is just unnecessary in this case. – Marc Glisse Aug 31 '15 at 07:12
  • @MarcGlisse Why? Class B has a huge vector inside it, wouldn't it be much better to move the contents instead of allocating memory twice? – AturSams Aug 31 '15 at 07:45
  • 1
    @zehelvion You are missing the point. It is better to construct things in the right place to begin with, so you don't have to copy or move them. – Marc Glisse Aug 31 '15 at 09:00
  • @MarcGlisse - No that's the unanswered point, how would you go about filling a vector with n instances of a class that has huge vectors as members without copying or moving? – AturSams Aug 31 '15 at 09:14
  • 2
    That's what I wrote above!!! You use emplace_back to build those instances directly in the vector (and obviously you call reserve(...) first to avoid reallocation, which would cause moves). – Marc Glisse Aug 31 '15 at 09:19
  • 1
    @zehelvion Here's a performance comparison: copying > moving > in-place construction. Moving is essentially an optimization of copy for rvalues, and in-place construction is the fastest if you already know where the constructed elements will end up. See my edited answer. – Emil Laine Aug 31 '15 at 10:37
  • @MarcGlisse ,didn't realize what you mean (until now). It sounded (to me) like you were suggesting moving is bad and copying is good and that emplace_back ensures copying. Didn't realize emplace was the solution I've been looking for all this time. Either way, ended up rolling a working copy and move constructor so it thought me a lot. Basically your comment is the answer (with it, we can explicitly forbid copy / move and still have a vector of instances of that class). – AturSams Aug 31 '15 at 12:32
4

No, std::vector takes ownership of its members, so the original code will work fine.

Chris Beck
  • 15,614
  • 4
  • 51
  • 87
3

std::vector copies the objects provided to it (by push_back() for example) and keeps them until the the vector itself is destroyed.

So the first code is totally OK as long as the copy constructor of B ( B(const B&) )is implemented properly.

Alex Lop.
  • 6,810
  • 1
  • 26
  • 45