Bug 68784 - deductible parameter type still requires explicit reference cast, e.g., std::thread
Summary: deductible parameter type still requires explicit reference cast, e.g., std::...
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.8.4
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-08 08:20 UTC by Wang Xuancong
Modified: 2015-12-08 16:02 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Wang Xuancong 2015-12-08 08:20:13 UTC
G++ compiler is weaker than MSVC on this point.
The following piece of code is compilable using MSVC but not in g++

include <iostream>
#include <unordered_map>
#include <thread>

using namespace std;

void thread_add(unordered_map<int, int>& ht, int from, int to)
{
    for(int i = from; i <= to; ++i)
        ht.insert(unordered_map<int, int>::value_type(i, 0));
}

void main()
{
    unordered_map<int, int> ht;
    thread t[2];

    t[0] = thread(thread_add, ht, 0, 9);
    t[1] = thread(thread_add, ht, 10, 19);

    t[0].join();
    t[1].join();

    std::cout << "size: " << ht.size() << std::endl;
}

error: no type named ‘type’ in ‘class std::result_of ...


g++ require explicit reference cast:
t[0] = thread(thread_add, ref(ht), 0, 9);


However, this is unnecessary because it can be easily deduced from the definition of the thread_add function whether to pass in a newly created copy or a reference to the existing copy.

I hope the compiler can be improved to resolve all these deducible parameter types so that programmers do not need to explicitly write the reference cast std::ref() every time, like what has been done in MSVC.
Comment 1 Jonathan Wakely 2015-12-08 10:24:31 UTC
Your version of MSVC does not follow the standard. std::thread copies its arguments and forwards them as rvalues, so an lvalue cannot bind to the non-const reference parameter. That's why you need to use std::ref.
Comment 2 Wang Xuancong 2015-12-08 10:48:29 UTC
All standards are made by people. No standard is perfect. If a standard causes more inconvenience to the users, then it is considered sub-optimal and has room for improvement.

Thus, unless you can give me a counter example that automatic parameter resolution can cause problems in some cases, I would consider the standard is not good enough, and will probably be fixed in some future C++ standards.

But since g++ is indeed following the standard, it should not be considered as a bug for now, formally speaking.
Comment 3 Jonathan Wakely 2015-12-08 11:22:06 UTC
(In reply to Wang Xuancong from comment #2)
> All standards are made by people. No standard is perfect. If a standard
> causes more inconvenience to the users, then it is considered sub-optimal
> and has room for improvement.

The behaviour mandated by the standard is by design, it is not a defect.
 
> Thus, unless you can give me a counter example that automatic parameter
> resolution can cause problems in some cases, I would consider the standard
> is not good enough, and will probably be fixed in some future C++ standards.

GCC's Bugzilla is not the right place to learn how C++ works.

It has nothing to do with deduction or "automatic parameter resolution", the error is because you cannot bind a non-const reference to an rvalue, similar to calling:

    thread_add(unordered_map<int, int>(), 0, 9);

This fails because you can't bind a temporary to the reference parameter (MSVC gets this right).

The standard requires std::thread to copy its arguments for safety reasons, to avoid silently forming dangling references to stack variables, MSVC gets that part right too. If you want a reference you must request it explicitly.

If you actually run your program after compiling it with MSVC you will see it prints:

size: 0

because 'ht' is never modified, because the threads insert into *copies* of the map, not the original. So even with MSVC your program doesn't do what you think it does. The standard requires you to use std::ref if you really want to create a thread with a reference to your variable.

The standard also requires the copied arguments to be forwarded as rvalues, to avoid surprising behaviour due to functions that take references modifying copies not the original objects. MSVC gets this wrong, and allows the copies to bind to lvalue reference parameters, and then your new threads modify the copies not the original, even though you think you've bound a reference.

> But since g++ is indeed following the standard, it should not be considered
> as a bug for now, formally speaking.

It's not a bug in any sense at all. The bug is in MSVC, I suggest you discuss it with Microsoft.
Comment 4 Jonathan Wakely 2015-12-08 11:28:15 UTC
(In reply to Wang Xuancong from comment #0)
> g++ require explicit reference cast:
> t[0] = thread(thread_add, ref(ht), 0, 9);

With that change your program has a data race due to modifying the same object in two separate threads, which leads to undefined behaviour. You should get a good book and read it carefully.
Comment 5 Ville Voutilainen 2015-12-08 11:39:06 UTC
And to add insult to injury, msvc accepts binding lvalue reference to temporaries, and chances are that their thread constructor does what it does partly because of that non-conforming core language extension.
Comment 6 Wang Xuancong 2015-12-08 15:08:29 UTC
You are right! I have tested myself. MSVC outputs 0 instead of crashing (crashing is what we expected), it is accepting compilation but not doing the job correctly, that it passes a temporarily created copy of the object instead of the reference of the original object.

Since g++ gives a compilation error, but MSVC accepts the syntax but fails silently. So in the end, g++ still better than MSVC, I take back my original claim.

However, I do not agree with "the standard requires std::thread to copy its arguments for safety reasons, to avoid silently forming dangling references to stack variables". Not all programmers are ignorant. Whether to pass in the reference of the original object or the temporarily created object, can be made clear by the programmer when they specify the arguments for the thread function, e.g.,
void thread_add(unordered_map<int, int>& ht, int from, int to);
void thread_add(unordered_map<int, int> ht, int from, int to);

The programmers should know what they are doing, and the difference between the above two thread function declarations. And they should also know that all local objects are stored on the stack, if the function returns, all local objects are destructed. Thus, the thread creator function can return only after all threads finished, if the thread function will ever access the creator function's local objects.
Comment 7 Jonathan Wakely 2015-12-08 16:02:04 UTC
That is a reasonable design, but it is not the design used by the C++11 standard library. This is not the right place to debate the merits of a very intentional decision made by a large group of experts after many months of discussion. Please discuss it in a more suitable forum.