[libcxx-commits] [PATCH] D100255: [libcxx] adds `range` access CPOs

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/ranges:1-3
+// #include <compare>              // see [compare.syn]
+// #include <initializer_list>     // see [initializer.list.syn]
+// #include <iterator>             // see [iterator.synopsis]
----------------
Please make the appropriate change to `libcxx/utils/generate_header_inclusion_tests.py`.
Also, this file needs a copyright header.


================
Comment at: libcxx/include/type_traits:2816
 typename decay<_Tp>::type
-__decay_copy(_Tp&& __t)
+__decay_copy(_Tp&& __t) _NOEXCEPT_(noexcept(_VSTD::forward<_Tp>(__t)))
 {
----------------
This is unconditionally noexcept. I don't think you intended that?


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:8-9
+//===----------------------------------------------------------------------===//
+#ifndef LIBCXX_TEST_LIBCXX_RANGES_RANGE_ACCESS_BAD_ACCESS_H
+#define LIBCXX_TEST_LIBCXX_RANGES_RANGE_ACCESS_BAD_ACCESS_H
+
----------------
Header guard doesn't match filename.


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:102-103
+
+// An otherwise okay "range", except that it isn't borrowable, and thus can't be accessed as an
+// rvalue
+class rvalue_range_unqualified_not_borrowable {
----------------
Throughout: please break lines to fit the review window. I suggest breaking after the last `,` (i.e., semantic linebreaks).


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:323-326
+namespace std::ranges {
+template <>
+inline constexpr bool enable_borrowed_range<unqualified_rvalue_range> = true;
+} // namespace std::ranges
----------------
FWIW, rather than reopening `namespace std` in the tests, I'd prefer to see
```
template <>
inline constexpr bool
    std::ranges::enable_borrowed_range<unqualified_rvalue_range> = true;
```


================
Comment at: libcxx/test/std/ranges/range.range/iterator_t.compile.pass.cpp:24
+
+namespace stdr = std::ranges;
+
----------------
Bikeshed: this seems much too easy to typo. I've been using `namespace rg = std::ranges` in my own slide code; anyone got thoughts?
Of course, I'm also not a huge fan of this test, since it has dependencies on so many containers. It would be better to do just like
```
#include <ranges>
#include <type_traits>
#include "test_iterators.h"
struct X {
    forward_iterator<int> begin();
    forward_iterator<int> end();
    forward_iterator<char> begin() const;
    forward_iterator<char> end() const;
};
static_assert(std::is_same_v<std::ranges::iterator_t<X>, forward_iterator<int>>);
static_assert(std::is_same_v<std::ranges::iterator_t<const X>, forward_iterator<char>>);
static_assert(std::is_same_v<std::ranges::iterator_t<double[10]>, double*>);
```


================
Comment at: libcxx/test/support/test_macros.h:292
 #define ASSERT_NOT_NOEXCEPT(...)
+#define CONSTEXPR_ASSERT(...)
 #else
----------------
I don't like this macro at all; I'd much rather you just write out
```
assert(test());
static_assert(test());
```
if that's what the test is supposed to be testing.
However, if @ldionne asks you to keep it for some reason, then please at least make it
`#define CONSTEXPR_ASSERT(...) assert((__VA_ARGS__))` in C++03 mode, and
```
#define CONSTEXPR_ASSERT(...) do { \
  static_assert((__VA_ARGS__), ""); \
  assert((__VA_ARGS__)); \
} while (0)
```
in C++11-and-later mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255