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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/ranges/range.range/iterator_t.compile.pass.cpp:24
+
+namespace stdr = std::ranges;
+
----------------
Quuxplusone wrote:
> 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*>);
> ```
> Bikeshed: this seems much too easy to typo. I've been using namespace `rg = std::ranges` in my own slide code; anyone got thoughts?

🤷 I'm happy to bikeshed on Discord (not in-review please).

> 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

Part of the point of this test is to ensure backwards compatibility with all of our containers. That was a design goal for ranges, and should be reflected here IMO.


================
Comment at: libcxx/test/support/test_macros.h:292
 #define ASSERT_NOT_NOEXCEPT(...)
+#define CONSTEXPR_ASSERT(...)
 #else
----------------
Quuxplusone wrote:
> 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.
> I don't like this macro at all; I'd much rather you just write out
> ```
> assert(test());
> static_assert(test());
> ```

That's pretty error-prone. One might forget one of the asserts, both of which are critical.

> `#define CONSTEXPR_ASSERT(...) assert((__VA_ARGS__))` in C++03 mode, and

Done.

> ```
> #define CONSTEXPR_ASSERT(...) do { \
>  static_assert((__VA_ARGS__), ""); \
>  assert((__VA_ARGS__)); \
> } while (0)
> ```
> in C++11-and-later mode.

I don't see why this change is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255