[libcxx-commits] [PATCH] D119677: [libc++] [test] Improve test coverage for std::{c, }{begin, end}.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

Nice cleanup! LGTM modulo some minor nits.



================
Comment at: libcxx/test/std/iterators/iterator.range/begin-end.pass.cpp:68
-//  initializer_list doesn't have cbegin/cend/rbegin/rend
-//  but std::cbegin(),etc work (b/c they're general fn templates)
-//     assert ( std::cbegin(c)  == c.cbegin());
----------------
I would prefer to keep these two lines of comment. Else using `il.begin()` instead of `il.cbegin()` in`assert(std::cbegin(cil) == il.begin());` looks wrong.

Same for the backward initializer list tests.

I agree with removing the commented out asserts.


================
Comment at: libcxx/test/std/iterators/iterator.range/begin-end.pass.cpp:97
+#if TEST_STD_VER > 11
+TEST_CONSTEXPR_CXX17 bool test_arrays_and_initializer_lists_backward()
+{
----------------
Why is this test `TEST_CONSTEXPR_CXX17` and the previous `TEST_CONSTEXPR_CXX14`?


================
Comment at: libcxx/test/std/iterators/iterator.range/begin-end.pass.cpp:155
+  ASSERT_SAME_TYPE(decltype(std::cend(c)), typename C::const_iterator);
+  assert(std::cbegin(c) == c.begin());
+  assert(std::cend(c) == c.end());
----------------
I like to see `std::cfoo() == c.cfoo()` in the tests. Testing against `c.foo()` looks wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119677