C++17 expression evaluation order and std::move

Question

Whilst refactoring some code today to change raw pointers to std::unique_ptr, I encountered a segmentation fault due to an order of evaluation bug.

The old code did something like the following:

void add(const std::string& name, Foo* f)
{
    _foo_map[name] = f;
}

void process(Foo* f)
{
    add(f->name, f);
}

The first, naive, refactoring of the code to use std::unique_ptr:

void add(const std::string& name, std::unique_ptr<Foo> f)
{
    _foo_map[name] = std::move(f);
}

void process(std::unique_ptr<Foo> f)
{
    add(f->name, std::move(f)); // segmentation-fault on f->name
}

The refactored code causes a segmentation fault, because the 2nd argument (std::move(f)) is processed first, and then the 1st argument (f->name) dereferences a moved-from variable, boom!

Possible fixes to this are to obtain a handle on Foo::name before moving it in the call to add:

void process(std::unique_ptr<Foo> f)
{
    const std::string& name = f->name;
    add(name, std::move(f));
}

Or perhaps:

void process(std::unique_ptr<Foo> f)
{
    Foo* fp = f.get();
    add(fp->name, std::move(f));
}

Both of these solutions require extra lines of code, and don't seem nearly as composable as the original (albeit UB) call to add.

Questions:

• Are either of the 2 proposed solutions above idiomatic C++, and if not, is there a better alternative?

• I see there are changes comingi n C++17 due to P0145R3 - Refining Expression Evaluation Order for Idiomatic C++. Will this change any of the above solutions / prevent their pitfalls?

Answer 1

For me these 2 proposals look bad. In either of those you are giving your Foo object away with the move. That means you can no longer make any assumptions about it's state after that. It could be deallocated inside the add function before the first argument (reference to the string or pointer to the object) is processed. Yes, it would work in the current implementation but it could break as soon as anyone touches the implementation of add or anything deeper in it.

The safe ways:

• Make a copy of the string and pass that as the first argument to add
• Refactor your add method that it only takes a single argument of type Foo and extracts Foo::name inside the method and doesn't take it as an argument. However you still have the same issue inside the add method.

In the second approach you should be able get around the evaluation order problem by first creating a new map entry (with default value) and getting a mutable reference to it and assigning the value afterwards:

auto& entry = _foo_map[f->name];
entry = std::move(f);

Not sure whether your map implementation supports getting a mutable reference to entries, but for many it should work.

If I think about it again you might also go for the "copy the name" approach. It needs to be copied anyway for the map key. If you copy it manually you can move it for the key there's no overhead.

std::string name = f->name;
_foo_map[std::move(name)] = std::move(f);

Edit:

As pointed out in the comments it should be possible to directly assign _foo_map[f->name] = std::move(f) inside the add function since the evaluation order is guaranteed here.

Answer 2

You could make add take the f by reference, which avoids an unnecessary copy/move (the same copy/move which trashes f->name):

void add(const std::string& name, std::unique_ptr<Foo> && f)
{
    _foo_map[name] = std::move(f);
}

void process(std::unique_ptr<Foo> f)
{
    add(f->name, std::move(f));
}

The foo_map[name] must be evaluated before operator= is called on it, so even if name refers to something depending on f there is no problem.