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

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

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
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()
+{
----------------
Mordante wrote:
> Why is this test `TEST_CONSTEXPR_CXX17` and the previous `TEST_CONSTEXPR_CXX14`?
`std::rbegin` was added in C++14, but `reverse_iterator` wasn't made constexpr-friendly until C++17. So `rbegin` went from nonexistent ('11) to runtime-only ('14) to fully constexpr ('17).


================
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());
----------------
Mordante wrote:
> I like to see `std::cfoo() == c.cfoo()` in the tests. Testing against `c.foo()` looks wrong.
Well, this is the flip side of your other comment about `il.begin()`. For every STL type that supports `cbegin`, it's an invariant that `c.begin() == c.cbegin()` and `c.end() == c.cend()`. But there are some STL types (e.g. `initializer_list`) that syntactically support only `c.begin()` and //not// `c.cbegin()`. So, if we were to try to use `x.cbegin()` everywhere in this test, we'd find there were some places that it wasn't supported and we'd have to fall back to `x.begin()`. Which, as you say, looks a bit wrong.
OTOH, if we just use `x.begin() / x.end()` consistently throughout (and `a` / `a + 3` for C arrays), then at least to //me// it doesn't look wrong at all. (Note my general rule that `cbegin` and `cend` are stupid, because we've had a perfectly fine `begin() const` and `end() const` since C++98. So I'm very used to just saying `.begin()` when I mean an iterator to the first element, and letting the compiler figure out whether it needs to const-qualify or not. I know some people do use `cbegin`, though.)

The other thing to notice here — not //directly// relevant but might affect your mental model — is that `std::cbegin(c)` does not in any way invoke `c.cbegin()` on the container. `std::cbegin(c)` actually invokes `std::begin(std::as_const(c))`, which invokes `c.begin() const`. So (if everything is piped together correctly) this code is basically testing that `c.begin() const == c.begin() non-const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119677