This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add ranges::in_fun_result
ClosedPublic

Authored by philnik on Jan 10 2022, 2:32 PM.

Details

Reviewers
Quuxplusone
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rG1e77b396ffe4: [libc++] Add ranges::in_fun_result
Summary

Add ranges::in_fun_result

Diff Detail

Event Timeline

philnik created this revision.Jan 10 2022, 2:32 PM
philnik requested review of this revision.Jan 10 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2022, 2:32 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/CMakeLists.txt
34

Here and modulemap, please alphabetize.
Good news: I'll render this nit unnecessary Real Soon Now via however D116958 ends up.

libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
21–27

Could this plausibly use the MoveOnly type from test/support/moveonly.h instead, just to reduce the line count a bit?
(Ditto consistently in the in_in_result PR, if it also applies there; and in @var-const's PR that includes in_out_result. I hadn't noticed this nit before.)

libcxx/test/std/algorithms/algorithms.results/no_unqiue_address.compile.pass.cpp
1 ↗(On Diff #398743)

Please fix the spelling of this file's filename, at some point (here or elsewhere or just as an NFC commit).

philnik updated this revision to Diff 398771.Jan 10 2022, 4:06 PM
philnik marked 2 inline comments as done.
  • Use MoveOnyl.h
  • alphabetize CMakeLists.txt and module.modulemap
philnik marked an inline comment as done.Jan 10 2022, 4:06 PM
philnik added inline comments.
libcxx/include/CMakeLists.txt
34

I've already looked at D116958. Thanks for that!

libcxx/test/std/algorithms/algorithms.results/no_unqiue_address.compile.pass.cpp
1 ↗(On Diff #398743)

This file was introduced in D116278. I'll fix it there.

ldionne edited reviewers, added: var-const; removed: ldionne.Jan 11 2022, 9:21 AM
philnik updated this revision to Diff 399950.Jan 14 2022, 3:33 AM
philnik marked an inline comment as done.
  • Use MoveOnly.h
  • alphabetize CMakeLists.txt and module.modulemap
philnik updated this revision to Diff 400171.Jan 14 2022, 3:46 PM
  • Rebased
  • Add constexpr test
philnik updated this revision to Diff 400636.Jan 17 2022, 1:45 PM
  • Add generated file
CaseyCarter added inline comments.
libcxx/include/__algorithm/in_fun_result.h
37

Shouldn't these be _VSTD-qualified?

libcxx/include/algorithm
702

This should be before in_in_result.h on line 702.

philnik updated this revision to Diff 400652.Jan 17 2022, 2:58 PM
philnik marked 2 inline comments as done.
  • Address comments
libcxx/include/algorithm
29

in_out_result isn't listed here; and in_in_out_result either isn't listed, or merge-conflicts.

libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
32

Same as on the in_in_out review, I think A would benefit from a comment about // no implicit conversion, and B could just be B(int);
In fact, all my comments from over there also apply here. Please add the exhaustive sets of MoveOnly tests, and so on and so forth. When you're done, diff <(sed s/in_fun/foo/g in_fun_result.pass.cpp) <(sed s/in_in/foo/g in_in_result.pass.cpp) should be basically an empty diff, and ~~~ <(sed s/in_in_out/foo/g in_in_out_result.pass.cpp) not much different.

var-const requested changes to this revision.Jan 24 2022, 2:14 PM
var-const added inline comments.
libcxx/include/__algorithm/in_fun_result.h
17

Does this file need the #pragma GCC system_header annotation?

23

Note: in in_out_result, I used more descriptive/verbose names like _InputIterator. Ideally these should be consistent. I think that having long names is more common in __algorithm/ (quick grepping shows ~360 hits for _InputIterator and 0 hits for _Ip in my current working copy).

30

_LIBCPP_HIDE_FROM_ABI?

libcxx/include/algorithm
29

in_out_result is at the bottom of synopsis (around line 655). I don't have a very strong opinion on where in the synopsis these should be listed, but they should all be together; I'd slightly prefer having them in the end because I feel that these are helper types that don't really need to be in the spotlight.

libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
81

Please add return 0;.

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

Can you please move the corresponding tests in in_out_result.compile.pass.cpp to this file for consistency? (not necessarily in this patch)

31

Would it be worthwhile to also test for the case of Empty and Empty2 where both classes are empty but they are distinct types?

This revision now requires changes to proceed.Jan 24 2022, 2:14 PM
var-const added inline comments.Jan 24 2022, 2:27 PM
libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
23–29

Optional: maybe move definitions of ConstructibleFrom and Empty down so that they're right above test() definition and thus closer to the point of usage?

61

Optional: consider adding a brief comment to each test case, e.g. Conversion via copy construction and Conversion via casting (also tests that move-only types can be converted).

70

Perhaps also test with a copy-only type? (that could be overkill)

72

Ultranit: please include <utility>.

philnik updated this revision to Diff 402814.Jan 25 2022, 2:48 AM
philnik marked 8 inline comments as done.
  • Use MoveOnyl.h
  • Add constexpr test
  • Add generated file
  • Address comments
  • Address comments
philnik updated this revision to Diff 404597.Jan 31 2022, 10:21 AM
  • Apply changes from in_in_out_result
philnik updated this revision to Diff 407709.Feb 10 2022, 3:43 PM
philnik marked an inline comment as done.
  • Rebased
  • use std
var-const accepted this revision.Feb 10 2022, 8:52 PM

LGTM modulo the remaining unresolved comments.

This revision is now accepted and ready to land.Feb 10 2022, 8:52 PM
philnik updated this revision to Diff 407850.Feb 11 2022, 5:10 AM
philnik marked 2 inline comments as done.
  • Fix CI
Quuxplusone accepted this revision.Feb 11 2022, 7:22 AM
Quuxplusone added inline comments.
libcxx/include/algorithm
29

@philnik thanks for moving in_out_result up! :)

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

s/10L/10/, right?

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

This was marked "Done", but not done. Since you moved the synopsis comment for in_out_result, I think it would be nice to also move the mentioned tests in this same commit. But if it happens in a followup, also cool.

philnik marked 3 inline comments as done.Feb 11 2022, 8:10 AM
philnik added inline comments.
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp
24

I'll do it in the results cleanup PR. The synopsis are just to easy to miss for my taste, so I moved them here.

This revision was landed with ongoing or failed builds.Feb 11 2022, 8:10 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.