[libcxx-commits] [PATCH] D113408: [libc++] Implement P0798R8 (Monadic operations for std::optional)

[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/std/utilities/optional/optional.monadic/and_then.pass.cpp:86-102
+constexpr void test_val_types() {
+  std::optional<int> i{};
+  const auto& ci = i;
+
+  i.and_then(LVal{});
+  ci.and_then(CLVal{});
+  std::move(i).and_then(RVal{});
----------------
I find this somewhat hard to follow. Instead, I'd have an easier time if things were grouped by the method overload you are testing:

```
constexpr void test_val_types() {
  // Test & overload
  {
    // Without & qualifier on F's operator()
    {
      std::optional<int> i{};
      i.and_then(LVal{});
    }

    // With & qualifier on F's operator()
    {
      std::optional<int> i{};
      RefQual l{};
      i.and_then(l);
    }
  }

  // Test const& overload
  {
    // Without & qualifier on F's operator()
    {
      std::optional<int> const i{};
      ci.and_then(CLVal{});
    }

    // With & qualifier on F's operator()
    {
        std::optional<int> const i{};
        const CRefQual cl{};
        ci.and_then(cl);
    }
  }

  // etc...
}
```

This applies everywhere else AFAICT. This is more verbose, however at the end of the day it's way easier to follow and see the pattern in the tests.

Other things that apply to all tests AFAICT:
- We are not testing the return type and return values of the functions.
- We are not testing with a `F` that returns an empty optional. Above, your `LVal` & friends always return an engaged optional.
- We are not exhaustively testing with a `i` that is engaged and non-engaged. Above, you only test with a disengaged optional.


================
Comment at: libcxx/test/std/utilities/optional/optional.monadic/and_then.pass.cpp:104-109
+constexpr void test_sfinae() {
+  std::optional<NonConst> opt{};
+  auto l = [](auto&& x) { return x.non_const(); };
+  opt.and_then(l);
+  std::move(opt).and_then(l);
+}
----------------
This needs a comment explaining what it's testing. `test_sfinae` isn't very descriptive, I'm staring at this and it's not immediately obvious to me what you're trying to test SFINAE for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113408