[libcxx-commits] [PATCH] D99141: [libcxx] adds `std::incrementable_traits` to <iterator>
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Apr 5 09:09:19 PDT 2021
Quuxplusone added inline comments.
================
Comment at: libcxx/include/__config:851
+#if _LIBCPP_STD_VER < 20 || defined(_LIBCPP_HAS_NO_CONCEPTS)
+#define _LIBCPP_HAS_NO_RANGES
----------------
Please use `<= 17`, for consistency with similar lines in this file. (We tend to guard C++2a stuff on `> 17`. I don't //think// there's anything special about the Ranges stuff in this respect, right?)
================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:34-35
+[[nodiscard]] constexpr bool check_incrementable_traits() noexcept {
+ constexpr bool result = check_difference_type_matches<T, Expected>;
+ static_assert(check_difference_type_matches<T const, Expected> == result);
+ return result;
----------------
Note that `T const` is the same type as `T` whenever `T` is a reference type. Therefore, any place in this file that instantiates `check_incrementable_traits<Foo&>` is suspicious — it's simply static-asserting that the difference type of `T` is the difference type of `T`.
I suggest that you add `static_assert(!std::is_reference_v<T>)` inside this template, and then re-run the tests, and investigate/fix any failures that arise from that.
I also suggest that if you actually expect `result` to be `true` in all cases, you should add
assert(result);
inside this function. (Or is it okay if `result` is false, sometimes? I didn't closely examine //all// the call sites.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99141/new/
https://reviews.llvm.org/D99141
More information about the libcxx-commits
mailing list