[libcxx-commits] [PATCH] D99855: [libcxx] makes `iterator_traits` C++20-aware
Zoe Carver via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Apr 13 13:59:42 PDT 2021
zoecarver added a comment.
The logic LGTM.
================
Comment at: libcxx/include/iterator:735
+template<__specifies_members _Ip>
+struct __iterator_traits<_Ip> {
+ using iterator_category = typename _Ip::iterator_category;
----------------
Can you define this //before// `iterator_traits`? I feel like there's needless complexity here.
================
Comment at: libcxx/include/iterator:758
+struct __iterator_traits<_Ip> {
+ using iterator_category = typename __iterator_traits_iterator_category<_Ip>::type;
+ using value_type = typename indirectly_readable_traits<_Ip>::value_type;
----------------
Again, is there any reason we can't define these //before// we use them?
================
Comment at: libcxx/include/iterator:874
+struct __iterator_traits {};
+#else
template <class _Iter, bool> struct __iterator_traits {};
----------------
Can you put a comment `// !defined...` here too?
================
Comment at: libcxx/include/iterator:916
+#endif
struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Tp*>
{
----------------
This also needs a few updated: `value_type` should now remove cv, adds `iterator_concept `, adds a const specialization. Are you making those changes in another patch?
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:82
+template <class I, class Wrapper>
+struct get_reference {
+ using type = std::iter_reference_t<Wrapper>;
----------------
I feel like both this and `get_pointer` are doing the thing where you write the same implementation twice and test that they do the same thing. IMHO that's not a great way to write tests.
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:107
+template <class I, class Wrapper, class Category>
+[[nodiscard]] constexpr bool check_wrapper_traits() {
+ using Traits = std::iterator_traits<Wrapper>;
----------------
Side note: the functions don't actually return anything useful. All the testing is done when the function is instantiated. So, why is this nodiscard? Wouldn't instantiating this function template inside a function be the same as instantiating it inside a static_assert?
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:125
+requires(sizeof...(Args) > 0) constexpr
+ void check_with_legacy_input_iterator() {
+ {
----------------
This indentation seems a bit odd, is this what clang format spit out? Either way, can you un-indent?
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:162
+template <class I>
+[[nodiscard]] constexpr bool check_with_legacy_iterator() {
+ {
----------------
This test I really like because I can read it and immediately see that it's doing the right thing. You say, "here is the input" and "here is the non-computed literal output."
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:251
+
+ inline static constexpr int value_type = 0;
+};
----------------
Nice. This is a good idea to test :)
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:289
+};
+static_assert(check_fails<reference_as_static_member>());
----------------
I have no idea what the consensus here was... are we still doing compile.passp.cpp tests or no? If no, then we should make this a "normal" .pass test.
================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
}
-
+#endif
{
----------------
Nit: `// _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99855/new/
https://reviews.llvm.org/D99855
More information about the libcxx-commits
mailing list