[libcxx-commits] [PATCH] D117780: [libc++][test] add vector<bool>::reference tests

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Thanks for doing this!



================
Comment at: libcxx/test/std/containers/sequences/vector.bool/reference/assign.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Naming-wise, IMO we should have `assign_bool.pass.cpp` and `assign_copy.pass.cpp`, or something along those lines: the important things are to put the name of the overload set //first// so that everything in the same overload set sorts together when you `ls`, and the parameter types explicitly afterward so that you can tell which file contains the overload you care about today.

Functionality-wise, I'd like to see a test that assigns false as well as true; and I'd like to see it come ready-made for constexprifying. Something like:
```
bool test() {
  std::vector<bool> vec;
  typedef std::vector<bool>::reference Ref;
  vec.push_back(true);
  vec.push_back(false);
  vec.push_back(true);
  vec.push_back(false);
  Ref r0 = vec[0];
  Ref r1 = vec[1];
  {
    vec[0] = false;
    vec[1] = true;
    r0 = r1;
    assert(vec[0]);
    assert(vec[1]);
  }
  {
    vec[0] = true;
    vec[1] = false;
    r0 = r1;
    assert(!vec[0]);
    assert(!vec[1]);
  }
  // And then repeat the same two tests for `std::move(r1)`?
  // And then repeat the same two tests for `static_cast<const Ref&>(r1)`??
  return true;
}

int main(int, char**) {
  test();
  // when constexpr vector is implemented, all we have to do is static_assert(test());

  return 0;
}
```
All your `main`s should end with `return 0;` as well as taking `argc, argv`, because freestanding wants that.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/reference/copy_ctor.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
`ctor_copy.pass.cpp` or something along those lines.

Btw, I think it'd be nice also to have some trivial-special-member tests, e.g.
```
LIBCPP_STATIC_ASSERT(!std::is_trivially_constructible<Ref>::value, "");
LIBCPP_STATIC_ASSERT(!std::is_trivially_copy_constructible<Ref>::value, "");
LIBCPP_STATIC_ASSERT(!std::is_trivially_move_constructible<Ref>::value, "");
LIBCPP_STATIC_ASSERT(!std::is_trivially_copy_assignable<Ref>::value, "");
LIBCPP_STATIC_ASSERT(!std::is_trivially_move_assignable<Ref>::value, "");
LIBCPP_STATIC_ASSERT(!std::is_trivially_destructible<Ref>::value, "");
```
just so we don't break those ABIs by accident. These can go into something like `reference/triviality.compile.pass.cpp`.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/reference/flip.pass.cpp:23
+  ref.flip();
+  assert(!ref);
+  ref.flip();
----------------
Let's test `!vec[0]` directly, and also test that e.g. `vec[1]` remains unmodified.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/reference/operator_bool.pass.cpp:24
+  assert(true_ref);
+  assert(!false_ref);
+}
----------------
Throw in a `static_assert(std::is_convertible<Ref, bool>::value, "");` somewhere near the top.
Test a simple `bool b = true_ref; assert(b);` because I believe all you're testing right now is //contextual// conversion to bool, and actually it looks like the standard requires //implicit// conversion, so we should test an implicit conversion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117780/new/

https://reviews.llvm.org/D117780