EricWF added a comment.
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`.
================
Comment at:
test/std/containers/sequences/array/array.tuple/get_const_rv.pass.cpp:32
@@ +31,3 @@
+ const C c = {std::unique_ptr<double>(new double(3.5))};
+ const T&& t = std::get<0>(std::move(c));
+ assert(*t == 3.5);
----------------
1. Please test the constexprness of this function in C++14 and beyond.
2. Please `static_assert` the return type using `decltype`. The assignment to
the expected type might hide conversions (though I doubt it in this case).
================
Comment at:
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.fail.cpp:30
@@ +29,3 @@
+ {
+ cref(std::get<0>(tup4()));
+ }
----------------
Add `// expected-error {{call to deleted function 'cref'}}` to the end of this
line to make the test use clang verify.
================
Comment at:
test/std/utilities/tuple/tuple.tuple/tuple.elem/get_const_rv.pass.cpp:26
@@ +25,3 @@
+struct Empty {};
+
+int main()
----------------
Same note as on the array test.
================
Comment at:
test/std/utilities/utility/pairs/pair.astuple/get_const_rv.pass.cpp:24
@@ +23,3 @@
+{
+#ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
+ {
----------------
I prefer using `// UNSUPPORTED: c++98, c++03` instead of conditional
compilation. That way LIT can report that the test was not run.
================
Comment at:
test/std/utilities/utility/pairs/pair.astuple/get_const_rv.pass.cpp:25
@@ +24,3 @@
+#ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
+ {
+ typedef std::pair<std::unique_ptr<int>, short> P;
----------------
Same note as the array and tuple tests.
================
Comment at:
test/std/utilities/utility/pairs/pair.astuple/pairs.by.type.pass.cpp:43
@@ -42,1 +42,3 @@
+ {
+ typedef std::unique_ptr<int> upint;
----------------
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.
http://reviews.llvm.org/D14839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
