[libc++][ranges] Add ranges::in_in_out_result

[philnik]

Summary

Add ranges::in_in_out_result

[CaseyCarter]

libcxx/include/__algorithm/in_in_out_result.h

35

The third convertible_to constraint is missing here, which indicates a lack of test coverage.

[philnik]

• Address comment

[philnik]

• Add no_unique_address tests

[• Quuxplusone]

LGTM % test comments, but I'll request changes anyway.

libcxx/include/__algorithm/in_in_out_result.h

29–33

Some whitespace issues here. Ditto below.

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

2

I don't see a CTAD test. How are we doing CTAD tests for these types? deduct.pass.cpp, or dump it in here, or what?

80

I don't think this buys you anything over a simple

std::ranges::in_in_out_result<long, float, int> res2 = res;
assert(res2.in1 == 10L);
~~~

(Also, moot point now, but ConstructibleFrom would be the wrong name for something that's implicitly convertible from; ConvertibleFrom would have been a better name.)

85–90

Since it was originally the constraint on _O1 that was missing, I think it makes sense to test all of the positions here. This can be as simple as

{
  std::ranges::in_in_out_result<MoveOnly, int, int> res{MoveOnly(1), 0, 0};
  auto res2 = std::ranges::in_in_out_result<MoveOnly, int, int>(std::move(res));
  assert(res.in1.get() == 0);  // it was moved-from
  assert(res2.in1.get() == 1);
  static_assert( std::is_constructible_v<std::ranges::in_in_out_result<MoveOnly, MoveOnly, MoveOnly>, std::ranges::in_in_out_result<int, int, int>&>);
  static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<MoveOnly, int, int>>);
  static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<int, MoveOnly, int>>);
  static_assert( std::is_move_constructible_v<std::ranges::in_in_out_result<int, int, MoveOnly>>);
  // Non-copyability of any element makes the whole thing non-copyable.
  static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<MoveOnly, int, int>>);
  static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<int, MoveOnly, int>>);
  static_assert(!std::is_copy_constructible_v<std::ranges::in_in_out_result<int, int, MoveOnly>>);
}

91–94

Please wrap these four lines in { } too, for consistency.

[philnik]

• Address comments

[philnik]

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

80

It's the same as in D116278.

[• Quuxplusone]

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

2

This is still unaddressed, right?
I think it would suffice to make an algorithms.results/deduct.compile.pass.cpp covering all the types, on the model of your existing no_unique_address.compile.pass.cpp.

ASSERT_SAME_TYPE(decltype(std::ranges::in_in_result(1, 'x')), std::ranges::in_in_result<int, char>);
ASSERT_SAME_TYPE(decltype(std::ranges::in_in_out_result(1, 'x', 1.f)), std::ranges::in_in_result<int, char, float>);

etc.

35–42

FWIW, I think lines 35-36 should just be B(int); — I don't see any realistic way for the behavior to differ between these two overload sets.
Line 29 could use a comment like // conversion is not implicit (to explain why line 31 is false not true).

80

Ah; consider removing it from both places here or in a followup. :)

99

You can remove [[maybe_unused]] now that the variable is used. (In general, [[maybe_unused]] hides missing assertions, so I'd advise, as a first approximation, never using it.)

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

33–35 ↗

(On Diff #402454)

I predict this getting cumbersome to read; I'd still rather see == 2 here, and ditto on line 28: basically pre-constant-fold everything we know to be a constant 1.

[philnik]

• Address comments

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

2

There are no deduction guides for the result types. I don't think this is supposed to work.

80

I meant that the implicit conversion does not work. Your example will not compile.

[Mordante]

In general it looks good but some minor issues.

libcxx/include/__algorithm/in_in_out_result.h

23

Nit, since there's only one out I would prefer to remove the 1 for the output. That also matches the wording of the Standard. Same for the rest of this header.

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

51

Do we need to test this? I don't see a requirement the conversion is noexcept(false).

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

34 ↗

(On Diff #402535)

Based on http://eel.is/c++draft/basic.memobj#intro.object-9 I expect 1 to be an valid answer. Actually I wonder how portable these answers are. The Standard states Two objects … may have the same address is uses may and not shall. This makes sense since compilers are allowed to ignore attributes. So I think this test should be moved from its current location to libcxx/test/libcxx/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

[• Quuxplusone]

libcxx/include/__algorithm/in_in_out_result.h

23

FWIW I'm cool with either way, but you can't use just _O because we don't do that. You could use _Out, I think would be the best option.

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

51

Good point, libraries are allowed to strengthen noexcept at will. Strike this (and similar lines throughout).

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

34 ↗

(On Diff #402535)

That's not a test/libcxx/ issue, though; that's a REQUIRES: clang issue! And we handle that by // XFAIL: msvc at the top of this file. So I think that's fine.

(The presence of the attribute on the data member itself is mandated for every conforming library, even if the MSVC compiler ignores the attribute.)

[philnik]

• Remove noexcept checks

libcxx/include/__algorithm/in_in_out_result.h

23

I made it _O1 to be consistent between in_in_result, in_in_out_result and in_out_out_result. I didn't write in_out_result, so that one is not consistent.

[Mordante]

libcxx/include/__algorithm/in_in_out_result.h

23

I think I read there will be some "cleanup" patch to unify all these result types correct?
If so I'm oke postponing this change until that patch.

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

34 ↗

(On Diff #402535)

I agree the adding of the attribute is required. But there's no portable way to check it since compilers are free to ignore attributes. I also am suspicious of the size of this specific object. Both Clang and GCC agree on 2 and due to ABI I don't expect it to change. But I can't see anything preventing another compiler to decide 1 is a nice size in this case. The two Empty types don't share the same address when sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == 1
Unlike the line above and below where the Empty's are next to each other.

[• Quuxplusone]

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

34 ↗

(On Diff #402535)

The two Empty types don't share the same address when sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == 1

Hmm, I don't think the compiler is permitted to lay out the second Empty object (or any object) at offset 1 if the sizeof the entire thing is also 1. However, I'm also aware that my mental model of [[no_unique_address]] is not yet correct, so I'm not sure you're wrong.
At least I think it is safe to keep this assert as == 2 until/unless we discover a platform where that assertion is incorrect, and then we can revisit it with that additional information. I don't think we should relax the assertion here based on hypotheticals that might never come to pass.

[philnik]

@Mordante Do you have any more complains or suggestions?

[Mordante]

LGTM!

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp

34 ↗

(On Diff #402535)

Sounds fair. It's at least very unlikely GCC and Clang change, since the change would be an ABI-break. Once MSVC supports [[no_unique_address]] we can evaluate the proper value for their implementation.

libcxx/include/CMakeLists.txt

set(files
   __algorithm/find_first_of.h
   __algorithm/find_if.h
   __algorithm/find_if_not.h
   __algorithm/for_each.h
   __algorithm/for_each_n.h
   __algorithm/generate.h
   __algorithm/generate_n.h
   __algorithm/half_positive.h
+  __algorithm/in_in_out_result.h
   __algorithm/in_in_result.h
   __algorithm/in_out_result.h
   __algorithm/includes.h
   __algorithm/inplace_merge.h
   __algorithm/is_heap.h
   __algorithm/is_heap_until.h
   __algorithm/is_partitioned.h
   __algorithm/is_permutation.h

libcxx/include/__algorithm/in_in_out_result.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___ALGORITHM_IN_IN_OUT_RESULT_H
+#define _LIBCPP___ALGORITHM_IN_IN_OUT_RESULT_H
+
+#include <__concepts/convertible_to.h>
+#include <__config>
+#include <__utility/move.h>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#ifndef _LIBCPP_HAS_NO_RANGES
+
+namespace ranges {
+template <class _I1, class _I2, class _O1>
+struct in_in_out_result {
+  [[no_unique_address]] _I1 in1;
+  [[no_unique_address]] _I2 in2;
+  [[no_unique_address]] _O1 out;
+
+  template <class _II1, class _II2, class _OO1>
+    requires convertible_to<const _I1&, _II1> && convertible_to<const _I2&, _II2> && convertible_to<const _O1&, _OO1>
+  _LIBCPP_HIDE_FROM_ABI constexpr operator in_in_out_result<_II1, _II2, _OO1>() const& {
+    return {in1, in2, out};
+  }
+
+  template <class _II1, class _II2, class _OO1>
+    requires convertible_to<_I1, _II1> && convertible_to<_I2, _II2> _LIBCPP_HIDE_FROM_ABI constexpr
+  operator in_in_out_result<_II1, _II2, _OO1>() && {
+    return {_VSTD::move(in1), _VSTD::move(in2), _VSTD::move(out)};
+  }
+};
+} // namespace ranges
+
+#endif // _LIBCPP_HAS_NO_RANGES
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___ALGORITHM_IN_IN_RESULT_H

libcxx/include/algorithm

 
 #include <initializer_list>
 
 namespace std
 {
 
 namespace ranges {
   template <class I1, class I2>
-    struct in_in_result; // since C++20
+    struct in_in_result;     // since C++20
+
+  template <class I1, class I2, class O>
+    struct in_in_out_result; // since C++20
 }
 
 template <class InputIterator, class Predicate>
     constexpr bool     // constexpr in C++20
     all_of(InputIterator first, InputIterator last, Predicate pred);
 
 template <class InputIterator, class Predicate>
     constexpr bool     // constexpr in C++20

 #include <__algorithm/find_first_of.h>
 #include <__algorithm/find_if.h>
 #include <__algorithm/find_if_not.h>
 #include <__algorithm/for_each.h>
 #include <__algorithm/for_each_n.h>
 #include <__algorithm/generate.h>
 #include <__algorithm/generate_n.h>
 #include <__algorithm/half_positive.h>
+#include <__algorithm/in_in_out_result.h>
 #include <__algorithm/in_in_result.h>
 #include <__algorithm/in_out_result.h>
 #include <__algorithm/includes.h>
 #include <__algorithm/inplace_merge.h>
 #include <__algorithm/is_heap.h>
 #include <__algorithm/is_heap_until.h>
 #include <__algorithm/is_partitioned.h>
 #include <__algorithm/is_permutation.h>

libcxx/include/module.modulemap

module __algorithm {
       module find_first_of            { private header "__algorithm/find_first_of.h" }
       module find_if                  { private header "__algorithm/find_if.h" }
       module find_if_not              { private header "__algorithm/find_if_not.h" }
       module for_each                 { private header "__algorithm/for_each.h" }
       module for_each_n               { private header "__algorithm/for_each_n.h" }
       module generate                 { private header "__algorithm/generate.h" }
       module generate_n               { private header "__algorithm/generate_n.h" }
       module half_positive            { private header "__algorithm/half_positive.h" }
+      module in_in_out_result         { private header "__algorithm/in_in_out_result.h" }
       module in_in_result             { private header "__algorithm/in_in_result.h" }
       module in_out_result            { private header "__algorithm/in_out_result.h" }
       module includes                 { private header "__algorithm/includes.h" }
       module inplace_merge            { private header "__algorithm/inplace_merge.h" }
       module is_heap                  { private header "__algorithm/is_heap.h" }
       module is_heap_until            { private header "__algorithm/is_heap_until.h" }
       module is_partitioned           { private header "__algorithm/is_partitioned.h" }
       module is_permutation           { private header "__algorithm/is_permutation.h" }

libcxx/test/libcxx/diagnostics/detail.headers/algorithm/in_in_out_result.module.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: modules-build
+
+// WARNING: This test was generated by 'generate_private_header_tests.py'
+// and should not be edited manually.
+
+// expected-error@*:* {{use of private header from outside its module: '__algorithm/in_in_out_result.h'}}
+#include <__algorithm/in_in_out_result.h>

libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
+// template <class I1, class I2, class O>
+// struct in_in_out_result;
+
+#include <algorithm>
+#include <cassert>
+#include <type_traits>
+
+#include "MoveOnly.h"
+
+template <class T>
+struct ConstructibleFrom {
+  constexpr ConstructibleFrom(T c) : content{c} {}
+  T content;
+};
+
+struct A {
+  explicit A(int);
+};
+static_assert(!std::is_constructible_v<std::ranges::in_in_out_result<A, A, A>,
+                                       std::ranges::in_in_out_result<int, int, int>>);
+
+struct B {
+  B(const int&);
+  B(int&&);
+};
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, std::ranges::in_in_out_result<int, int, int>>);
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, std::ranges::in_in_out_result<int, int, int>&>);
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, const std::ranges::in_in_out_result<int, int, int>>);
+static_assert(std::is_constructible_v<std::ranges::in_in_out_result<B, B, B>, const std::ranges::in_in_out_result<int, int, int>&>);
+
+struct C {
+  C(int&);
+};
+static_assert(!std::is_constructible_v<std::ranges::in_in_out_result<C, C, C>, std::ranges::in_in_out_result<int, int, int>&>);
+
+static_assert(std::is_convertible_v<std::ranges::in_in_out_result<int, int, int>&,
+                                    std::ranges::in_in_out_result<long, long, long>>);
+static_assert(!std::is_nothrow_convertible_v<std::ranges::in_in_out_result<int, int, int>&,
+                                             std::ranges::in_in_out_result<long, long, long>>);
+static_assert(std::is_convertible_v<const std::ranges::in_in_out_result<int, int, int>&,
+                                    std::ranges::in_in_out_result<long, long, long>>);
+static_assert(!std::is_nothrow_convertible_v<const std::ranges::in_in_out_result<int, int, int>&,
+                                             std::ranges::in_in_out_result<long, long, long>>);
+static_assert(std::is_convertible_v<std::ranges::in_in_out_result<int, int, int>&&,
+                                    std::ranges::in_in_out_result<long, long, long>>);
+static_assert(!std::is_nothrow_convertible_v<std::ranges::in_in_out_result<int, int, int>&&,
+                                             std::ranges::in_in_out_result<long, long, long>>);
+static_assert(std::is_convertible_v<const std::ranges::in_in_out_result<int, int, int>&&,
+                                    std::ranges::in_in_out_result<long, long, long>>);
+static_assert(!std::is_nothrow_convertible_v<const std::ranges::in_in_out_result<int, int, int>&&,
+                                             std::ranges::in_in_out_result<long, long, long>>);
+
+constexpr bool test() {
+  {
+    std::ranges::in_in_out_result<int, double, float> res{10L, 0., 1.f};
+    assert(res.in1 == 10);
+    assert(res.in2 == 0.);
+    assert(res.out == 1.f);
+    std::ranges::in_in_out_result<ConstructibleFrom<int>, ConstructibleFrom<double>, ConstructibleFrom<float>> res2 = res;
+    assert(res2.in1.content == 10);
+    assert(res2.in2.content == 0.);
+    assert(res2.out.content == 1.f);
+  }
+  {
+    std::ranges::in_in_out_result<MoveOnly, int, int> res{MoveOnly{}, 0, 0};
+    assert(res.in1.get() == 1);
+    [[maybe_unused]] auto res2 = static_cast<std::ranges::in_in_out_result<MoveOnly, int, int>>(std::move(res));
+    assert(res.in1.get() == 0);
+  }
+  auto [in1, in2, out] = std::ranges::in_in_out_result<int, int, int>{1, 2, 3};
+  assert(in1 == 1);
+  assert(in2 == 2);
+  assert(out == 3);
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}