Find the Bug—Andrzej Krzemieński

Save to:
Instapaper Pocket Readability

Can you spot the bug?

Find the Bug

by Andrzej Krzemieński

From the article:

Today, let's take a short test. Find what is likely to be a bug in the following code and suggest how to fix it.

void Catalogue::populate(vector<string> const& names)
{
  vec_.clear();
  vec_.resize(names.size());

  for (size_t i = 0; i < names.size(); ++i)
    vec_[i] = make_unique<Entry>(names[i]);
}

Add a Comment

Comments are closed.

Comments (3)

0 1

Haitham Gad said on Feb 17, 2014 04:02 PM:

"names" are passed in by reference. When you assign the elements of vec_, unique pointers to the strings inside "names", the unique pointers become the owners of these strings. If for any reason, the lifetime of the passed in names vector extends beyond that of vec_, all the elements inside the passed in names vector will have been deleted.
0 0

Nick Hounsome said on Feb 19, 2014 04:37 AM:

It is not exception safe.
"resize" can throw an exception after clearing vec_.
The idiomatic solution is "copy and swap".
Assuming vec_ is vector<Entry> we would create a local vector<Entry> temp.fill it as shown and then swap(vec_,temp). The "clear" is not needed.
The swap method cannot throw and thus we either succeed or leave vec_ unchanged.

P.S. Haitham - You are mistaken - the strings are passed to the constructor of Entry which would be something like Entry(const string&) - vec_ contains unique_ptr<Entry> - There is no issue of ownership.
0 0

ahhz said on Feb 19, 2014 10:55 AM:

If names is an rvalue, you would want the strings to be presented to make_unique<Entry> as rvalue too. However, this will not happen automatically and as a consequence (bug) the Entry constructor is likely to make a redundant copy of the strings.
As a solution, do two things: (1) pass names by value, (2) move names[i] into make_unique<Entry>.
void Catalogue::populate(vector<string> names)
vec_[i] = make_unique<Entry>(move(names[i]));

If you also wish to deal with the exception guarantee issue, do the following:
void Catalogue::populate(vector<string> names)
{
decltype(vec_) tmp;
tmp.reserve(names.size());
for (auto && name : names)
tmp.push_back(make_unique<Entry>(move(name)));
swap(tmp, vec_);
}


Update: It is actually not certain that the Entry constructor needs a copy of the string value. If not, my solution is worse than the original. So it may be best to have an overloaded function for rvalue names, and this was then not strictly a bug.

But, in fairness, what could the constructor of Entry possibly do with a const string& if not to copy it?