[PATCH] [libcxx] Add <experimental/any> v2.

Marshall Clow mclow.lists at gmail.com
Mon Mar 16 13:01:17 PDT 2015


Reviewed section any.cons


================
Comment at: test/std/experimental/any/any.class/any.cons/copy.pass.cpp:34
@@ +33,3 @@
+    {
+        small const s(1);
+        any const a(s);
----------------
I guess I'm OK with using `large` and `small` here (instead of `int` and `complex<long double>`, say), but I think that would be better.

If you're going to explicitly check small object optimizations, that should not be in the "test/std/" hierarchy.

================
Comment at: test/std/experimental/any/any.class/any.cons/copy_throws.pass.cpp:24
@@ +23,3 @@
+{
+    using std::experimental::any;
+    {
----------------
Again; `large` and `small`.

================
Comment at: test/std/experimental/any/any.class/any.cons/default.pass.cpp:35
@@ +34,3 @@
+        assert(a.type() == typeid(void));
+        assert(globalMemCounter.checkOutstandingNewEq(0));
+    }
----------------
Do you want to assert this before the `any` is created as well?

================
Comment at: test/std/experimental/any/any.class/any.cons/move.pass.cpp:42
@@ +41,3 @@
+    // small object
+    {
+        small s(1);
----------------
same comment as for copy-constructor for `large` vs. `small`

================
Comment at: test/std/experimental/any/any.class/any.cons/move_throws.pass.cpp:39
@@ +38,2 @@
+    assert(throws_on_move::count == 0);
+}
----------------
Looks good.

================
Comment at: test/std/experimental/any/any.class/any.cons/non_copyable_value.fail.cpp:26
@@ +25,3 @@
+    non_copyable() {}
+    non_copyable(non_copyable &&) {}
+};
----------------
Shouldn't this really be `non_copyable & non_copyable(non_copyable &&) { return *this; }`  ??  

and similar for the declaration of the copy constructor.

I find it hard to write good "compile failing" tests. Frequently they fail for the wrong reasons.

================
Comment at: test/std/experimental/any/any.class/any.cons/value.pass.cpp:55
@@ +54,3 @@
+        small s(1);
+        any const a(static_cast<small &&>(s));
+        assert(!a.empty());
----------------
Not `std::move(s)` ?

================
Comment at: test/std/experimental/any/any.class/any.cons/value.pass.cpp:98
@@ +97,3 @@
+        large l(1);
+        any const a(static_cast<large &&>(l));
+        assert(!a.empty());
----------------
See line #55

================
Comment at: test/std/experimental/any/any.class/any.cons/value_copy_throws.pass.cpp:53
@@ +52,2 @@
+    assert(large_throws_on_copy::count == 0);
+}
----------------
Looks good.

================
Comment at: test/std/experimental/any/any.class/any.cons/value_move_throws.pass.cpp:26
@@ +25,3 @@
+    {
+        throws_on_move v;
+        assert(throws_on_move::count == 1);
----------------
You have a `small_throws_on_copy` and a `large_throws_on_copy`; but only a single `throws_on_move`.

Why the difference?

http://reviews.llvm.org/D6762

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list