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

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/test/libcxx/ranges/range.access/access.h:134
+  [[nodiscard]] constexpr bool faulty_##cpo() {                                                                        \
+    static_assert(!std::ranges::__##cpo::__member_##cpo<T&>);                                                          \
+    static_assert(!std::ranges::__##cpo::__member_##cpo<T const&>);                                                    \
----------------
I'm not seeing the point of testing those as libc++-only tests. Is it not possible to get equivalent coverage without reaching into the entrails of libc++?

We normally try to only create libc++-specific tests for libc++-specific behavior (like a vendor extension or some specific guarantee that we provide but isn't mandated by the spec).


================
Comment at: libcxx/test/support/test_macros.h:292
 #define ASSERT_NOT_NOEXCEPT(...)
+#define CONSTEXPR_ASSERT(...)
 #else
----------------
cjdb wrote:
> 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.
Honestly I think defining a `CONSTEXPR_ASSERT` macro is much more error-prone than just spelling it out. I'm generally totally in favour of eliminating duplication, but not when that duplication is both minimal and helpful to see explicitly. I'd rather not add `CONSTEXPR_ASSERT`, I think it obfuscates things more than anything else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255