Implement LWG2485, get() should be overloaded for const tuple&&.
Details
Diff Detail
Event Timeline
Added initial review comments.
I need to look a little more into how we form the rvalue reference to the returned element in the case where the element is an lvalue reference. I think it's currently correct but I just want to double check. I think the new language is a bit vague when it says get returns a reference to the element.
test/std/containers/sequences/array/array.tuple/get_const_rv.pass.cpp | ||
---|---|---|
33 |
| |
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.fail.cpp | ||
31 | Add // expected-error {{call to deleted function 'cref'}} to the end of this line to make the test use clang verify. | |
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.pass.cpp | ||
27 | Same note as on the array test. | |
test/std/utilities/utility/pairs/pair.astuple/get_const_rv.pass.cpp | ||
25 | I prefer using // UNSUPPORTED: c++98, c++03 instead of conditional compilation. That way LIT can report that the test was not run. | |
26 | Same note as the array and tuple tests. | |
test/std/utilities/utility/pairs/pair.astuple/pairs.by.type.pass.cpp | ||
70 | This test seems wrong to me. The name of the file suggests we are testing the "by-type" overloads but your tests use indexes with get. It seems like the test above yours does the same thing as well. Also please add constexpr tests and tests for the second type as well. |
To expand on my concerns about lvalue elements, I would like to see tests something like this:
int x = 42; int const y = 43; std::pair<int&, int const&> const p(x, y); static_assert(std::is_same<int const&, decltype(std::get<0>(std::move(p)))>::value, ""); static_assert(std::is_same<int const&, decltype(std::get<1>(std::move(p)))>::value, "");
I assume you agree that this test has the correct behavior?
I believe the first static_assert is incorrect, const is shallow so it should be just int&.
Pair and array look good. Still have to look at tuple.
Could you add test that check the overloads are actually noexcept as well?
include/utility | ||
---|---|---|
139 | Can you add the by-type overloads to the synopsis? I noticed the current overloads are missing as well, so if your feeling kind can you add those? | |
test/std/containers/sequences/array/array.tuple/get_const_rv.pass.cpp | ||
21 | #include "test_macros.h" | |
test/std/utilities/utility/pairs/pair.astuple/get_const_rv.pass.cpp | ||
24 | This needs #include "test_macros.h" to get TEST_STD_VER | |
test/std/utilities/utility/pairs/pair.astuple/pairs.by.type.pass.cpp | ||
19 | Can you TEST_STD_VER this while your here? |
LGTM after the requested changes. Thanks.
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.fail.cpp | ||
---|---|---|
30 | Could you add a comment about why this test is important? On it's face it looks a little silly but it's really the rational for this entire change. | |
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.pass.cpp | ||
25 | #include "test_macros.h" | |
50 | Test for int&& and int const&& as well please. | |
test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.pass.cpp | ||
21 | You can remove this guard. The test is already supported for C++11 and earlier. | |
71 | Same as above: Rvalue ref tests. | |
test/std/utilities/utility/pairs/pair.astuple/get_const_rv.pass.cpp | ||
38 | Same as above: Rvalue ref tests. | |
test/std/utilities/utility/pairs/pair.astuple/pairs.by.type.pass.cpp | ||
59 | rvalue ref tests please. |
Can you add the by-type overloads to the synopsis? I noticed the current overloads are missing as well, so if your feeling kind can you add those?