Page MenuHomePhabricator

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

Authored by philnik on Jan 17 2022, 2:01 PM.

Details

Summary

Add ranges::in_in_out_result

Diff Detail

Event Timeline

philnik created this revision.Jan 17 2022, 2:01 PM
philnik requested review of this revision.Jan 17 2022, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2022, 2:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
CaseyCarter added inline comments.
libcxx/include/__algorithm/in_in_out_result.h
36

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

philnik updated this revision to Diff 400657.Jan 17 2022, 3:24 PM
philnik marked an inline comment as done.
  • Address comment
philnik updated this revision to Diff 400660.Jan 17 2022, 3:33 PM
  • Add no_unique_address tests
ldionne edited reviewers, added: var-const; removed: ldionne.Jan 20 2022, 3:58 PM
Quuxplusone requested changes to this revision.Jan 23 2022, 7:45 AM

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.

This revision now requires changes to proceed.Jan 23 2022, 7:45 AM
philnik updated this revision to Diff 402454.Jan 24 2022, 3:35 AM
philnik marked 3 inline comments as done.
  • Address comments
philnik added inline comments.Jan 24 2022, 3:43 AM
libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp
80

It's the same as in D116278.

Quuxplusone added inline comments.
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

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 updated this revision to Diff 402535.Jan 24 2022, 8:07 AM
philnik marked 3 inline comments as done.
  • 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.

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

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

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

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 updated this revision to Diff 402809.Jan 25 2022, 2:23 AM
philnik marked 2 inline comments as done.
  • 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 added inline comments.Jan 26 2022, 10:45 AM
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

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.

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

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 marked 11 inline comments as done.Jan 29 2022, 5:07 AM

@Mordante Do you have any more complains or suggestions?

Mordante accepted this revision.Jan 30 2022, 5:43 AM

LGTM!

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

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.

This revision is now accepted and ready to land.Jan 30 2022, 5:43 AM
This revision was automatically updated to reflect the committed changes.