This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] add vector<bool>::reference tests
ClosedPublic

Authored by philnik on Jan 20 2022, 5:53 AM.

Details

Summary

Add test coverage for vector<bool>::reference

Diff Detail

Event Timeline

philnik requested review of this revision.Jan 20 2022, 5:53 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 5:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Jan 20 2022, 7:21 AM

Thanks for doing this!

libcxx/test/std/containers/sequences/vector.bool/reference/assign.pass.cpp
1 ↗(On Diff #401613)

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 mains should end with return 0; as well as taking argc, argv, because freestanding wants that.

libcxx/test/std/containers/sequences/vector.bool/reference/copy_ctor.pass.cpp
1 ↗(On Diff #401613)

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.

libcxx/test/std/containers/sequences/vector.bool/reference/flip.pass.cpp
24

Let's test !vec[0] directly, and also test that e.g. vec[1] remains unmodified.

libcxx/test/std/containers/sequences/vector.bool/reference/operator_bool.pass.cpp
25

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.

This revision now requires changes to proceed.Jan 20 2022, 7:21 AM
philnik updated this revision to Diff 401641.Jan 20 2022, 7:54 AM
philnik marked 4 inline comments as done.
  • Address comments

LGTM % a couple last nits!

libcxx/test/std/containers/sequences/vector.bool/reference/assign_bool.pass.cpp
25

Here as well, please test with both true and false.

libcxx/test/std/containers/sequences/vector.bool/reference/ctor_copy.pass.cpp
23

This test feels insufficient — it's just testing that ref2 refers to some true boolean, not that it refers to the same vec[0] "object" — but I have no great ideas for what would be better here. (We want to test &ref == &ref2 but of course that doesn't work because vector<bool>.)

libcxx/test/std/containers/sequences/vector.bool/reference/flip.pass.cpp
24

IMO you could remove assert(!ref) and assert(ref) at this point (lines 22-24, 26, 30), but this is fine too.

philnik updated this revision to Diff 402856.Jan 25 2022, 4:59 AM
philnik marked 3 inline comments as done.
  • Address comments
philnik updated this revision to Diff 404272.Jan 29 2022, 7:56 AM
  • Rebased
  • Fix CI
libcxx/test/std/containers/sequences/vector.bool/reference/ctor_copy.pass.cpp
23

I guess what you could do here is

Ref ref2 = ref;
assert(ref == ref2 && ref2 == true);
ref.flip();
assert(ref == ref2 && ref2 == false);

No biggie.

philnik updated this revision to Diff 404281.Jan 29 2022, 9:00 AM
philnik marked an inline comment as done.
  • Update ctor_copy.pass.cpp
ldionne accepted this revision.Jan 31 2022, 8:27 AM

I only skimmed but this looks OK. I'm surprised we didn't have any tests for that. Please just make sure you looked carefully to avoid duplicating tests that might already exist, but if that's really adding coverage, thanks a lot for doing this!

This revision is now accepted and ready to land.Jan 31 2022, 8:27 AM
This revision was automatically updated to reflect the committed changes.