This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by philnik on Dec 25 2021, 5:28 AM.

Details

Summary

Add std::ranges::in_in_result

Diff Detail

Event Timeline

philnik created this revision.Dec 25 2021, 5:28 AM
philnik requested review of this revision.Dec 25 2021, 5:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2021, 5:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 396194.Dec 25 2021, 5:40 AM
  • Added <algorithm> changes
Quuxplusone requested changes to this revision.Dec 25 2021, 8:50 PM
Quuxplusone added a subscriber: ldionne.
Quuxplusone added inline comments.
libcxx/include/__algorithm/results.h
10–11 ↗(On Diff #396194)

This is the right header guard, but the wrong everything else. ;) I'd like to see this in a file named __algorithm/in_in_result.h.
It would also be useful to see a dependent PR (as a "child revision" of this one) implementing some algorithm that actually uses this type. std::ranges::swap_ranges would be a good candidate.

libcxx/include/algorithm
20–38

Defer to @ldionne's opinion on this, but I wonder if it would suffice to replace lines 20–38 with

template <class I1, class I2>
  struct ranges::in_in_result;  // since C++20

Going into the full level of detail for every single result type here in this synopsis comment is going to take hundreds of lines, for very little benefit that I can discern.

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
39

Please make this simply

std::ranges::in_in_result<double, int> res2 = res;
assert(res2.in1 == 10);
assert(res2.in2 == 0);

For simplicity/realism, and also because you need to test that the conversion is non-explicit.

You're missing tests to verify (1) that convertible_to is not constructible_from, e.g.

struct A { explicit A(int); };
static_assert(!std::is_constructible_v<std::ranges::in_in_result<A, A>, std::ranges::in_in_result<int, int>>);

and (2) that the copy constructor can still be used when the move-constructor can't, e.g.

struct B { B(const int&); B(int&&) = delete; };
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, std::ranges::in_in_result<int, int>>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, std::ranges::in_in_result<int, int>&>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, const std::ranges::in_in_result<int, int>>);
static_assert(std::is_constructible_v<std::ranges::in_in_result<B, B>, const std::ranges::in_in_result<int, int>&>);

struct C { C(int&); };
static_assert(!std::is_constructible_v<std::ranges::in_in_result<C, C>, std::ranges::in_in_result<int, int>&>);

(Here I'm using !is_constructible to prove that there's not just "not an implicit ctor", but also not an explicit ctor either.)

50

I'd also like to see tests that the conversion operator is unconditionally noexcept(false).
(No conditional-explicit, no 4x overloads for perfect forwarding, no conditional noexcept... These result types are strangely old-school, compared to most Ranges stuff.)

This revision now requires changes to proceed.Dec 25 2021, 8:50 PM
philnik updated this revision to Diff 396221.Dec 26 2021, 5:41 AM
philnik marked 2 inline comments as done.
  • Renamed results.h to in_in_result.h
  • Updated test
libcxx/include/__algorithm/results.h
10–11 ↗(On Diff #396194)

I see you want me to do more work :). I'll add one soon.

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
39

You can't cast to std::ranges::in_in_result<double, int>, because that would be narrowing int to double and double to int, which is not allowed here IIUC.

50

Do you want more than checking that all const and & possibilities are checked to be noexcept(false) or is that enough?

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
39

libstdc++ thinks in_in_result<int, double> is convertible to in_in_result<double, int> https://godbolt.org/z/WaeTMq6T5 and it makes intuitive sense to me that it should be convertible, because int is convertible to double and vice versa.
But Microsoft STL produces the SFINAE-unfriendly narrowing diagnostic you were worried about: https://godbolt.org/z/qTqsYKn9c
Do you feel like digging into libstdc++ and figuring out whether they're doing something blatantly problematic to make this work? libstdc++'s behavior certainly feels nicer to me: whenever the conversion operator exists, it ought to compile. If we followed MSVC's lead here, we'd basically be lying to std::is_convertible_v.

50

What you have below seems to me like enough.

philnik added inline comments.Dec 26 2021, 2:04 PM
libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
39

It seems the only difference is that GCC disables conversion warnings: https://godbolt.org/z/TvfTsjP5d. The implementation itself is exactly the same.

Quuxplusone added a reviewer: ldionne.

LGTM % nitpicks.

libcxx/include/CMakeLists.txt
29–31

Swap lines 30 and 31: inp < in_ < is_ seems good to me.

libcxx/include/__algorithm/in_in_result.h
13–14

Swap these two lines. (One of these days I'll make graph_header_deps.py just check for a-z order.)

libcxx/include/algorithm
20

This namespace formatting is going to be weird no matter what we do, but I suggest just adding a blank line between lines 19 and 20.

695

Swap with line 694: inp < in_ < is_ seems good to me.

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
14

Remove this #include of a private header. (The Modules build should be unhappy with this, right?)

philnik updated this revision to Diff 398088.Jan 7 2022, 2:57 AM
philnik marked 8 inline comments as done.
  • Addressed comments
libcxx/include/__algorithm/in_in_result.h
13–14

You know there is a clang-tidy check for that, right? Maybe we should just enforce the clang-tidy checks at some point?

philnik updated this revision to Diff 398112.Jan 7 2022, 5:43 AM
  • Added module test
Quuxplusone added inline comments.Jan 7 2022, 6:54 AM
libcxx/include/__algorithm/in_in_result.h
13–14

Just posted D116809! If you have the ability to run clang-tidy locally, it'd be interesting to see what it says about that branch. Like, is clang-tidy happy with that branch, or would it still complain — and if so, do you think its remaining complaints are valid?

philnik updated this revision to Diff 398153.Jan 7 2022, 8:12 AM
  • Added UNSUPPORTED comments
philnik updated this revision to Diff 398210.Jan 7 2022, 12:03 PM
  • XFAIL on msvc
libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
14–15

I just checked the build results, and it looks like the only problem with this test is that it checks something like sizeof(in_in_result<Empty, int>) == sizeof(int), which we expect to fail on MSVC. So you should just guard that one test under an appropriate #if, or IMO even better, split that test case out into in_in_result.no_unique_address.pass.cpp and XFAIL only that file. At the same time you can expand the test coverage to test sizeof(in_in_result<int, Empty>) and sizeof(in_in_result<Empty, Empty>) too!

38
philnik updated this revision to Diff 398259.Jan 7 2022, 3:34 PM
philnik marked 2 inline comments as done.
  • Move [[no_unique_address]] test into its own file
Quuxplusone added inline comments.Jan 9 2022, 2:23 AM
libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
75–80

Nit: If you s/Empty/int/ on lines 75–77, then you don't need line 34 at all in this file anymore.

philnik updated this revision to Diff 398763.Jan 10 2022, 3:48 PM
  • Use MoveOnly.h
ldionne edited reviewers, added: var-const; removed: ldionne.Jan 11 2022, 9:23 AM
ldionne added inline comments.
libcxx/include/algorithm
20–38

Yeah, I think just mentioning the forward declaration of in_in_result is enough, like we appear to be doing in the current version of the patch.

Mostly looks good, but there are some remaining issues.

libcxx/include/__algorithm/in_in_result.h
37
libcxx/include/module.modulemap
250

This should be one line below, like in the header.

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
61

Can you also test the contents of main as constexpr ?

75
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp
17 ↗(On Diff #398763)

Since this test is in std instead of libcxx I think it would be good to mention the class may only have the two members specified in the standard.
http://eel.is/c++draft/algorithms.results#1

24 ↗(On Diff #398763)

Why 2 * sizeof(Empty)?

libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp
17 ↗(On Diff #398763)

@Mordante: Are you basically asking to replace line 16 with

// [algorithms.results]/1
//   Each of the class templates specified in this subclause has the template parameters,
//   data members, and special members specified below, and has no base classes or members
//   other than those specified.

? (If so: I like that idea!)

@philnik: The fact that the standard defines exactly two members, both public, probably means that we're mandated to support structured binding:

[[maybe_unused]] auto [in1, in2] = std::ranges::in_in_result<int, int>{1, 2};

Consider adding that simple test case to in_in_result.pass.cpp. (Attn: @var-const, re in_out_result.pass.cpp, for consistency.)

24 ↗(On Diff #398763)

Because of the "empty-base exclusion principle".
FWIW, I would have just said == 2); since sizeof-an-empty-class is 1 by definition AFAIK.

Mordante added inline comments.Jan 11 2022, 11:43 AM
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp
17 ↗(On Diff #398763)

Yes and adding a structured bindings test is a good idea!

24 ↗(On Diff #398763)

Seems there's always a blog post :-)
I thought [[no_unique_address]] allows it to be 1. Reading your blog and looking in the standard I see
the answer in [intro.object]/9 "Two objects with overlapping lifetimes that are not bit-fields may have the same address if one is nested within the other, or if at least one is a subobject of zero size and they are of different types; otherwise, they have distinct addresses and occupy disjoint bytes of storage."

Thanks.

philnik updated this revision to Diff 399511.Jan 12 2022, 4:37 PM
philnik marked 11 inline comments as done.
  • Address comments
philnik added inline comments.Jan 12 2022, 4:38 PM
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp
24 ↗(On Diff #398763)

I thought 2 * sizeof(Empty) would be clearer than just 2.

Mordante accepted this revision.Jan 13 2022, 8:42 AM

LGTM with one suggestion.

libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
73

Maybe remove the [[maybe_unused]] and test whether in1 and in2 have the proper values.

This revision is now accepted and ready to land.Jan 13 2022, 8:42 AM
philnik updated this revision to Diff 399821.Jan 13 2022, 4:01 PM
philnik marked an inline comment as done.
  • Address comment
This revision was automatically updated to reflect the committed changes.