[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