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

Christopher Di Bella via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Apr 26 14:39:50 PDT 2021


cjdb added a comment.

In D100255#2717763 <https://reviews.llvm.org/D100255#2717763>, @zoecarver wrote:

> Okay, to be honest, I really do not like these tests.
>
> I think we need to workshop the structure here because the current tests fall down in terms of readability and demonstrability. We can get to correctness and completeness later, but let's get the model right first.
>
> Tests shouldn't have too much boilerplate, and in the test file itself, it should be clear what is being tested and how it's being tested. There should be enough context to figure out what's going on.
>
> The current tests in `ranges/range.access/*` fail at this. There is a lot of boilerplate. We should try to be modest with the use of macros and only use them when they are unequivocally the best answer, and there isn't another clear way to do something. In these tests //everything, including the main function//, is hidden inside of a macro. This is not readable.
>
> A good test would look like this:
>
>   struct a_clearly_named_type;
>   static_assert(!is_invocable_v<RangesBeginT, a_clearly_named_type>);
>
> There are a few things here: first, it is clear what is being tested because of the context. The name `CHECK_MEMBER_ACCESS` tells me very little. However, the expression `!is_invocable_v` tells me that we shouldn't be able to call `ranges::begin` because of some constraint. Hopefully, the type `a_clearly_named_type` explains what constraint is being violated.
>
> Second, it's easily verifiable. If I'm reading this test with the standard next to me, I can easily match the text in the standard to the `static_assert` and check it off mentally. It is much more difficult to do this when you have to follow the input into a macro full of boilerplate and distractions where the code is generated. My mind does not have a large enough cache for this to be feasible, sorry :P
>
> It would be one thing if these macros were stress testing concepts, or ranges, or whatever. But they're not. They're being used for the tests that are supposed to demonstrate the API and its correctness.
>
> The issues above are making it hard for me to verify correctness, but these tests are definitely not complete. For example, where are the tests for a type without any members? Or a member with the wrong return type? I think there are a few tests you need to add; luckily, I think these will be immediately obvious with a new test structure.

I agree, will look into this.



================
Comment at: libcxx/include/__ranges/begin.h:30
+
+namespace ranges {
+  template <class _Tp>
----------------
zoecarver wrote:
> Nit: You keep opening and closing this namespace but everything in this file goes in it. Why not just open it at the beginning and leave it open?
I think @ldionne wants stuff to be indented inside this namespace, and I don't want to do more than one indentation.


================
Comment at: libcxx/include/__ranges/begin.h:36
+  template<class _Tp>
+  concept __is_complete = requires { sizeof(_Tp); };
+} // namespace ranges
----------------
zoecarver wrote:
> Seems like it might be helpful to have this in a more general place, WDYT?
Where do you suggest?


================
Comment at: libcxx/include/__ranges/begin.h:64
+      constexpr bool __complete = __is_complete<remove_cv_t<_Tp> >;
+      if constexpr (__complete) { // used to disable cryptic diagnostic
+        return __t + 0;
----------------
zoecarver wrote:
> I'm confused, why can't we just do 
> ```
> static_assert(__is_complete<remove_cv_t<_Tp> >, "...");
> return __t + 0;
> ```
> ?
`__t + 0` yields a cryptic diagnostic (that could also be a red herring) when the condition is false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255



More information about the libcxx-commits mailing list