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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255