You may write:
template<class... TS>
Item& create(TS&&... mArgs)
{
items.emplace_back(std::make_unique<Item>(std::forward<TS>(mArgs)...));
return *items.back();
}
Вопрос
I found out (thanks to a StackOverflow comment) a security flaw in my code:
std::vector<std::unique_ptr<Item>> items;
template<class... TS> Item& create(TS&&... mArgs)
{
auto item(new Item(std::forward<TS>(mArgs)...);
items.emplace_back(item); // Possible exception and memory leak
return *item;
}
Basically, allocating the Item
with a raw new
may leak memory if the emplace_back
throws.
The solution is never using a raw new
, but using a std::unique_ptr
right in the method body.
std::vector<std::unique_ptr<Item>> items;
template<class... TS> Item& create(TS&&... mArgs)
{
auto item(std::make_unique<Item>(std::forward<TS>(mArgs)...);
items.emplace_back(std::move(item));
return *item; // `item` was moved, this is invalid!
}
As you can see, returning item
is invalid, as I had to move item
using std::move
to emplace it into the items
container.
I can't think of a solution that requires storing item
's address in an additional variable. The original (buggy) solution is however very concise and easy to read.
Is there a more elegant way of returning an std::unique_ptr
that was moved for emplacement in a container?
Решение
You may write:
template<class... TS>
Item& create(TS&&... mArgs)
{
items.emplace_back(std::make_unique<Item>(std::forward<TS>(mArgs)...));
return *items.back();
}
Другие советы
Caching the reference before the emplace is a more generally applicable option (e.g., for non-vector containers):
template<class... TS> Item& create(TS&&... mArgs)
{
auto item = std::make_unique<Item>(std::forward<TS>(mArgs)...);
auto& foo = *item;
items.emplace_back(std::move(item));
return foo; // This *is* valid.
}
Your question is tagged C++11, and other answers suggesting make_unique
fail to mention that it is a C++14 feature. I believe this C++11 approach also resolves the leak.
#include <vector>
#include <memory>
#include <utility>
#include <iostream>
struct Item
{
int a, b;
Item(int aa, int bb) : a{aa}, b{bb} { }
};
static std::vector<std::unique_ptr<Item>> items;
template <class... Ts> Item& create(Ts&&... args)
{
items.emplace_back(std::unique_ptr<Item>{new Item(std::forward<Ts>(args)...)});
return *items.back();
}
int main()
{
Item& x = create(1, 2);
std::cout << "( " << x.a << ", " << x.b << " )" << std::endl;
}
This should be safe because emplace_back()
cannot be called until a unique_ptr<Item>
was already constructed, so even if emplace_back()
does throw, your Item
is already managed by the unique_ptr
.