2

This program's output is missing the intended values for name and Some string.

Name: , Age: 4, Some string:

Name: , Age: 3, Some string:

If I dynamically allocate memory for each item for myVector in Source.cpp then it works out okay. I'm just wondering why this works out as it does, and what mistakes I have made.

Worth noting is I'm using Visual Studio 2015 C++ compiler.

Parent.h

#pragma once

#include <string>

class Parent
{
public:
    explicit Parent(std::string name, int age)
        : m_name(name), m_age(age) {};

    virtual const std::string toString() const
    {
        return "Name: " + m_name + ", Age: " + std::to_string(m_age);
    }

private:
    std::string m_name;
    int m_age;
};

Child.h

#pragma once

#include "Parent.h"

class Child : public Parent
{
public:
    explicit Child(std::string name, int age, std::string someString)
        : Parent(name, age), m_someString(someString) {};

    virtual const std::string toString() const
    {
        return Parent::toString() + ", Some string: " + m_someString;
    }
private:
    std::string m_someString;
};

Source.cpp

#include <vector>
#include <iostream>

#include "Parent.h"
#include "Child.h"

int main()
{
    std::vector<Parent*> myVector;

    myVector.push_back(&Child("Foo", 4, "Test"));
    myVector.push_back(&Child("Bar", 3, "Test"));

    for (auto p : myVector)
    {
        std::cout << p->toString() << std::endl;
    }

    return 0;
}
Community
  • 1
  • 1
B. Lee
  • 191
  • 6
  • 3
    Why `myVector.push_back(&Child("Foo", 4, "Test"));` and not `myVector.push_back(new Child("Foo", 4, "Test"));`? – DimChtz Aug 04 '17 at 23:53
  • 2
    Yup the memory lifecycle of Child doesnt persist beyond the line its being used. Ie. Child constructed. Add a reference to it when you copy it. Child destructed. The list contains destructed Child references. Think about using smart pointers or copy constructors so Child continues to exist beyond the line. – Stephen Quan Aug 04 '17 at 23:59

2 Answers2

3

When you write

myVector.push_back(&Child("Foo", 4, "Test"));

you create a temporary object of type Child and save its pointer in the vector.

But it's a temporary object that is immediately destroyed when push_back() ends; so the value of the pointer memorized in the vector point to an area of memory that is free and, probably, recycled.

When you use this pointer

std::cout << p->toString() << std::endl;

the behavior is undefined.

Is different when you allocate the memory of the object because the object isn't destroyed immediately after the push_back() call and the object is still available when you call toString() [but remember to call delete, at the end, or (better) use smart pointers].

-- EDIT --

You ask for

care to add how smart pointers fit in this problem

so I show a simple simplified example using std::unique_ptr.

Observe that you have to use emplace_back() instead of push_back(); this is because the isn't a copy constructor (for evident reasons) for std::unique_ptr [anyway, I suggest to use emplace_back(), when possible, instead push_back()].

#include <vector>
#include <memory>
#include <iostream>

struct foo
 { std::string str; };

int main ()
 {
   std::vector<std::unique_ptr<foo>> myV;

   myV.emplace_back(new foo{"one"});
   myV.emplace_back(new foo{"two"});

   for ( auto const & p : myV )
      std::cout << p->str << std::endl;

   return 0;
 }

-- EDIT 2 --

As pointed by Daniel Schepler (thanks!), adding values in the std::vector with

myV.emplace_back(new foo{"one"});

can generate a memory leak in case the new element in myV cause a new allocation of the internal memory of the vector and this allocation fail.

It's an unlikely case but it's possible so I think it's better to avoid the risk (the paranoia is an asset).

A way to solve this problem is call reserve() before emplace_back(), so there is no need to reallocate the internal memory after the new

myV.reserve(2U); // or more
myV.emplace_back(new foo{"one"});
myV.emplace_back(new foo{"two"});

but, to solve this sort of problem, C++14 introduced std::make_unique, so in C++14 can be used together push_back() (but this example require a foo(std::string const &) constructor for the struct foo)

myV.push_back(std::make_unique<foo>("one"));
myV.push_back(std::make_unique<foo>("two"));

Another way, also for C++11, is moving in myV a temporary std::unique_ptr

myV.push_back(std::unique_ptr<foo>{ new foo{"one"} });
myV.push_back(std::unique_ptr<foo>{ new foo{"two"} });

so can be called the move constructor of std::unique_ptr and, in case of failure reallocating the vector, the destructor of the temporary std::unique_ptr free the allocated memory.

max66
  • 65,235
  • 10
  • 71
  • 111
  • Ah, building upon the answer from sg7, this gives me a complete understanding. So the proper way would be to dynamically allocate with `new`, and then invoke free(p) in my loop, for example. Moreover, care to add how smart pointers fit in this problem, or perhaps link some resource where I can read about them? – B. Lee Aug 05 '17 at 00:03
  • 2
    @B.Lee do not use `free` use `delete` – sg7 Aug 05 '17 at 00:05
  • 1
    @B.Lee A good discussion on Smart Pointers: [What is a smart pointer and when should I use one?](https://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one) – user4581301 Aug 05 '17 at 00:11
  • 1
    @B.Lee - answer improved. – max66 Aug 05 '17 at 00:19
  • 1
    `myV.emplace_back(new foo{"one"})` has a potential memory leak if the vector needs to reallocate for more storage but then that allocation fails (and some caller catches the `bad_alloc` exception). `myV.push_back(std::make_unique("one"))` doesn't have that issue. – Daniel Schepler Aug 05 '17 at 00:27
  • @DanielSchepler - I don't understand why a failed relocation of the vector should give a memory leak when the `std::unique_ptr` is allocated via `new` and should avoid it when allocated with `std::make_unique`; as far I know, the rationale behind `std:make_unique` is another; anyway, `push_back(std::make_unique("one"))` doesn't compile (do you mean `emplace_back()`?) because there isn't a copy constructor for `std::unique_ptr`. – max66 Aug 05 '17 at 00:36
  • 1
    If the allocation of expanded vector backing storage fails, then the `emplace_back` returns wthout ever constructing a `unique_ptr` from the given pointer. Whereas if you pass in an rvalue reference to an existing `unique_ptr`, then the stack unwinding would call the destructor for the temporary, which frees the memory since the move constructor was never called. – Daniel Schepler Aug 05 '17 at 20:59
  • 1
    And are you sure `myV.push_back(std::make_unique("one"))` doesn't compile? As far as I know, it should - `std::vector::push_back` has an overload taking an rvalue reference. – Daniel Schepler Aug 05 '17 at 21:01
  • @DanielSchepler - you're right about the failure expanding vector case; I din't think about it; and your're right also about the `push_back()` case: doesn't compile for another reason: `foo` (in my minimal example) is without a constructor that receive a `std::string`; if you add this constructor, `push_back()` compile. – max66 Aug 06 '17 at 18:15
  • @DanielSchepler - improved the answer pondering about your observation. Thanks. – max66 Aug 06 '17 at 18:41
2

When you use std::vector<Parent*> myVector; vector of pointers you have to allocate memory for the object:

   myVector.push_back( new Child("Foo", 4, "Test"));

To avoid dynamic creation of objects use:

std::vector<Parent> myVector

   myVector.push_back(Child("Foo", 4, "Test"));

Since you are asking how to get rid of the allocated objects this is a suggestion below:

clearAndDestroy(&myVector);

And the function is:

template <typename V>
void clearAndDestroy( std::vector<V*> *&myV)
{
    if(myV == NULL)
        return;

    std::set<V*> mySet;

    typename std::vector<V*>::iterator itr;
    typename std::set<V*>::iterator sitr;

    itr = myV->begin();
    while (itr != myV->end())
    {
        mySet.insert(*itr);
        ++itr;
    }

    sitr = mySet.begin();
    while (sitr != mySet.end())
    {
        delete(*sitr);
        ++sitr;
    }

    myV->clear(); // Removes all elements from the vector leaving the container with a size of 0.
}
sg7
  • 6,108
  • 2
  • 32
  • 40