Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is std::move safe in an arguments list when the argument is forwarded, not move constructed?

Tags:

Trying to provide a solution to std::string_view and std::string in std::unordered_set, I'm playing around with replacing std::unordered_set<std::string> with std::unordered_map<std::string_view, std::unique_ptr<std::string>> (the value is std::unique_ptr<std::string> because the small string optimization would mean that the address of the string's underlying data will not always be transferred as a result of std::move).

My original test code, that seems to work, is (omitting headers):

using namespace std::literals;

int main(int argc, char **argv) {
    std::unordered_map<std::string_view, std::unique_ptr<std::string>> mymap;

    for (int i = 1; i < argc; ++i) {
        auto to_insert = std::make_unique<std::string>(argv[i]);
        
        mymap.try_emplace(*to_insert, std::move(to_insert));
    }

    for (auto&& entry : mymap) {
        std::cout << entry.first << ": " << entry.second << std::endl;
    }

    std::cout << std::boolalpha << "\"this\" in map? " << (mymap.count("this") == 1) << std::endl;
    std::cout << std::boolalpha << "\"this\"s in map? " << (mymap.count("this"s) == 1) << std::endl;
    std::cout << std::boolalpha << "\"this\"sv in map? " << (mymap.count("this"sv) == 1) << std::endl;
    return EXIT_SUCCESS;
}

I compile with g++ 7.2.0, compile line is g++ -O3 -std=c++17 -Wall -Wextra -Werror -flto -pedantic test_string_view.cpp -o test_string_view receiving no warnings of any kind, then run, getting the following output:

$ test_string_view this is a test this is a second test
second: second
test: test
a: a
this: this
is: is
"this" in map? true
"this"s in map? true
"this"sv in map? true

which is what I expected.

My main concern here is whether:

        mymap.try_emplace(*to_insert, std::move(to_insert));

has defined behavior. The *to_insert relies on to_insert not being emptied (by move constructing the std::unique_ptr stored in the map) until after the string_view is constructed. The two definitions of try_emplace that would be considered are:

try_emplace(const key_type& k, Args&&... args);

and

try_emplace(key_type&& k, Args&&... args);

I'm not sure which would be chosen, but either way, it seems like key_type would be constructed as part of calling try_emplace, while the arguments to make the mapped_type (the "value", though maps seem to use value_type to refer to the combined key/value pair) are forwarded along, and not used immediately, which makes the code defined. Am I correct in this interpretation, or is this undefined behavior?

My concern is that other, similar constructions that seem definitely undefined still seem to work, e.g.:

mymap.insert(std::make_pair<std::string_view,
                            std::unique_ptr<std::string>>(*to_insert,
                                                          std::move(to_insert)));

produces the expected output, while similar constructs like:

mymap.insert(std::make_pair(std::string_view(*to_insert),
                            std::unique_ptr<std::string>(std::move(to_insert))));

trigger a Segmentation fault at runtime, despite none of them raising warnings of any kind, and both constructs seemingly equally unsequenced (unsequenced implicit conversions in the working insert, unsequenced explicit conversion in the segfaulting insert), so I don't want to say "try_emplace worked for me, so it's okay."

Note that while this question is similar to C++11: std::move() call on arguments' list, it's not quite a duplicate (it's what presumably makes the std::make_pair here unsafe, but not necessarily applicable to try_emplace's forwarding based behavior); in that question, the function receiving the arguments receives std::unique_ptr, triggering construction immediately, while try_emplace is receiving arguments for forwarding, not std::unique_ptr, so while the std::move has "occurred" (but done nothing yet), I think we're safe since the std::unique_ptr is constructed "later".

like image 775
ShadowRanger Avatar asked Aug 08 '18 17:08

ShadowRanger


People also ask

When should we use std :: move?

std::move is used to indicate that an object t may be "moved from", i.e. allowing the efficient transfer of resources from t to another object. In particular, std::move produces an xvalue expression that identifies its argument t . It is exactly equivalent to a static_cast to an rvalue reference type.

Does std forward move?

C++ std::move does not move and std::forward does not forward. This article dives deep into a long list of rules on lvalues, rvalues, references, overloads and templates to be able to explain a few deceivingly simple lines of code using std::move and std::forward.

Why do we need std :: forward?

std::forward helps to implement perfect forwarding. This mechanism implies that objects passed to the function as lvalue expressions should be copied, and objects passed to the function as rvalue expressions should be moved. If you assign an rvalue reference to some ref variable, then ref is a named entity.

Where is std :: move defined?

Move Constructor And Semantics: std::move() is a function used to convert an lvalue reference into the rvalue reference. Used to move the resources from a source object i.e. for efficient transfer of resources from one object to another. std::move() is defined in the <utility> header.


1 Answers

Yes, your call to try_emplace is perfectly safe. std::move actually doesn't move anything, it just casts the passed variable to an xvalue. No matter what order the arguments are initialized, nothing will ever be moved because the parameters are all references. References bind directly to objects, they do not call any constructors.

If you look at your second snippet, you'll see that std::make_pair also takes its parameters by reference, so in that case too a move will not be made except in the constructor body.

Your third snippet however does have the problem of UB. The difference is subtle, but if the arguments of make_pair are evaluated left-to-right, then a temporary std::unique_ptr object will get initialized with the moved from value of to_insert. This means that now, to_insert is null because a move actually happened because you are explicitly constructing an object that actually performs the move.

like image 149
Rakete1111 Avatar answered Oct 11 '22 14:10

Rakete1111