I have the following classes:
Bar.hpp
#include <string>
class Bar
{
public:
Bar(const std::string& info);
std::string getInfo() { return info; }
protected:
private:
std::string info;
};
Bar.cpp:
#include <Bar.hpp>
Bar::Bar(const std::string& info)
: info(info) { }
Foo.hpp:
#include <string>
#include <vector>
#include <Bar.hpp>
class Foo
{
public:
static void printInfoStore();
static Foo* getFoo(const std::string& name);
Foo() {};
std::string getName() { return name; }
std::vector<Bar> getBars();
void addBar(const std::string& info);
protected:
private:
static std::vector<Foo> infoStore;
Foo(const std::string& name);
std::vector<Bar> bars{};
std::string name;
};
Foo.cpp
#include <iostream>
#include <Foo.hpp>
Foo::Foo(const std::string& name)
: name(name) { }
//static
std::vector<Foo> Foo::infoStore(0);
//static
void Foo::printInfoStore() {
std::cout << "InfoStore is { ";
for (Foo& foo : infoStore) {
std::cout << foo.getName() << "=[ ";
for (Bar& bar : foo.getBars()) {
std::cout << bar.getInfo() << " ";
}
std::cout << "] ";
}
std::cout << " }" << std::endl;
}
//static
Foo* Foo::getFoo(const std::string& name) {
for (Foo& foo : infoStore) {
if (foo.getName() == name) {
return &foo;
}
}
Foo* foo = new Foo(name);
infoStore.push_back(*foo);
return foo;
}
std::vector<Bar> Foo::getBars() {
return bars;
}
void Foo::addBar(const std::string& info) {
Bar* bar = new Bar(info);
bars.push_back(*bar);
}
Basically there is a static vector that holds multiple Foo objects each of which has a vector of Bar objects.
Then I have the following main.cpp:
#include <Foo.hpp>
#include <Bar.hpp>
int main(int argc, char *argv[])
{
Foo::printInfoStore();
Foo::getFoo("Foo1")->addBar("info11"); // info11 is not added to Foo1
Foo::printInfoStore();
Foo::getFoo("Foo1")->addBar("info12");
Foo::printInfoStore();
Foo::getFoo("Foo1")->addBar("info13");
Foo::printInfoStore();
Foo::getFoo("Foo2")->addBar("info21"); // info21 is not added to Foo2
Foo::printInfoStore();
Foo::getFoo("Foo2")->addBar("info22");
Foo::printInfoStore();
return 0;
}
The output is the following:
InfoStore is { }
InfoStore is { Foo1=[ ] }
InfoStore is { Foo1=[ info12 ] }
InfoStore is { Foo1=[ info12 info13 ] }
InfoStore is { Foo1=[ info12 info13 ] Foo2=[ ] }
InfoStore is { Foo1=[ info12 info13 ] Foo2=[ info22 ] }
The strange thing is that the first Bars added for each Foo object (info11 and info21) do not get added. My guess is that there might be a second initialization of the bars vector that happens after the parent Foo object but I don't know if this is the case, nor I can find a rationale behind it.
I tried to initialize the bars vector explicitly within the foo constructor but to no avail: the first Bar added is always discarded.
So why does it happen? What's wrong with my code? What can be done to avoid that behavior?
CodePudding user response：
All of your news are causing memory leaks. None of this code should be using new at all, since none of the vectors are holding pointers.
For that matter, since getFoo() always returns a valid object, whether an existing object or newly-pushed object, it should return a Foo& reference to that object rather than a Foo* pointer (you can return a pointer if you really want to, but I would not advise it).
Either way, you would have to make sure that whatever you do return actually refers to an object that is inside of your vector. Your output is not what you expect because the code is NOT doing this correctly, which is the root of your problem.
When you call getFoo() for a non-existent object, you create a new object, then push a copy of that object into your vector, and then return a pointer to the new'ed object, not a pointer to the copied object. So, any values you subsequently store in the new'ed object appear to be discarded when you print the vector later, since the values don't exist in the copied object that is actually inside of the vector. When you call getFoo() again for an existing object, you return a pointer to the copied object that is inside the vector, and you do not create a new object.
On a similar note, getBars() should also return a reference to its vector, not return a copy of its vector by value.
With all of that said, try something more like this instead:
Bar.hpp
#include <string>
class Bar
{
public:
Bar(const std::string& info);
std::string getInfo() const;
private:
std::string info;
};
Bar.cpp:
#include <Bar.hpp>
Bar::Bar(const std::string& info)
: info(info) { }
std::string Bar::getInfo() const
{ return info; }
Foo.hpp:
#include <string>
#include <vector>
#include <Bar.hpp>
class Foo
{
public:
static void printInfoStore();
static Foo& getFoo(const std::string& name);
std::string getName() const;
std::vector<Bar>& getBars();
const std::vector<Bar>& getBars() const;
void addBar(const std::string& info);
private:
static std::vector<Foo> infoStore;
std::vector<Bar> bars;
std::string name;
Foo(const std::string& name);
};
Foo.cpp
#include <iostream>
#include <Foo.hpp>
Foo::Foo(const std::string& name)
: name(name) { }
//static
std::vector<Foo> Foo::infoStore;
//static
void Foo::printInfoStore() {
std::cout << "InfoStore is { ";
for (const Foo& foo : infoStore) {
std::cout << foo.getName() << "=[ ";
for (const Bar& bar : foo.getBars()) {
std::cout << bar.getInfo() << " ";
}
std::cout << "] ";
}
std::cout << " }" << std::endl;
}
//static
Foo& Foo::getFoo(const std::string& name) {
for (Foo& foo : infoStore) {
if (foo.getName() == name) {
return foo;
}
}
infoStore.push_back(Foo(name));
return infoStore.back();
// or, in C 11..14:
// infoStore.emplace_back(name);
// return infoStore.back();
// or, in C 17 onward:
// return infoStore.emplace_back(name);
}
std::string Foo::getName() const {
return name;
}
std::vector<Bar>& Foo::getBars() {
return bars;
}
const std::vector<Bar>& Foo::getBars() const {
return bars;
}
void Foo::addBar(const std::string& info) {
bars.push_back(Bar(info));
// or, in C 11 onward:
// bars.emplace_back(info);
}
main.cpp
#include <Foo.hpp>
#include <Bar.hpp>
int main(int argc, char *argv[])
{
Foo::printInfoStore();
Foo::getFoo("Foo1").addBar("info11");
Foo::printInfoStore();
Foo::getFoo("Foo1").addBar("info12");
Foo::printInfoStore();
Foo::getFoo("Foo1").addBar("info13");
Foo::printInfoStore();
Foo::getFoo("Foo2").addBar("info21");
Foo::printInfoStore();
Foo::getFoo("Foo2").addBar("info22");
Foo::printInfoStore();
return 0;
}
CodePudding user response：
You store copy in your vector (with memory leaks).
Fixed version:
Foo* Foo::getFoo(const std::string& name) {
for (Foo& foo : infoStore) {
if (foo.getName() == name) {
return &foo;
}
}
return &infoStore.emplace_back(Foo{name});
}
void Foo::addBar(const std::string& info) {
bars.emplace_back(info);
}