This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] Create a test tool `ProxyIterator` that customises `iter_move` and `iter_swap`
ClosedPublic

Authored by huixie90 on Jul 4 2022, 5:18 PM.

Details

Summary

It is meant to be used in ranges algorithm tests.
It is much simplified version of C++23's tuple + zip_view.
Using std::swap would cause compilation failure and using std::move would not create the correct rvalue proxy which would result in copies.

Diff Detail

Event Timeline

huixie90 created this revision.Jul 4 2022, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 5:18 PM
huixie90 requested review of this revision.Jul 4 2022, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 5:18 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Jul 5 2022, 12:40 AM

Thanks for working on this! Could you go through the list of already implemented algorithms and instantiate every one of those with at least ProxyIterator<int*>? If there are broken ones, please add the line commented out with a // TODO: Fix proxy iterator above it (to make it easier to grep for). It would also be nice if you could add ProxyIterator<input_iterator<int*>> through ProxyIterator<contiguous_iterator<int*>> to the permutation tests (since we know at least some of them are broken).

libcxx/test/support/test.support/test_proxy.pass.cpp
2

I don't object to ProxyIterator having it's own test, but I wouldn't ask for one either.

18

Use MoveOnly.h?

libcxx/test/support/test_proxy.h
42 ↗(On Diff #442155)

Please don't use implementation details. This makes our tests non-portable.

134 ↗(On Diff #442155)

Should this call base recursively?

161–165 ↗(On Diff #442155)

Can't you just = default this?

266 ↗(On Diff #442155)

Could you add a few static_asserts here to ensure that ProxyIterator meets the correct concepts? (That would be enough testing IMO)

This revision now requires changes to proceed.Jul 5 2022, 12:40 AM
huixie90 added inline comments.Jul 5 2022, 3:19 AM
libcxx/test/support/test_proxy.h
42 ↗(On Diff #442155)

do you have any alternative suggestions for __copy_cvref_t? I don't want to copy the implementations here.

134 ↗(On Diff #442155)

Could you please clarify? I thought my implementation does it recursively. Are you suggesting not to do it recursively?

266 ↗(On Diff #442155)

There are few reason I wrote the test:

  1. It took me few iterations to get all the things correct and having the tests is really helpful for me to make sure the implementation is correct.
  1. This header doesn't depend on those test iterators atm which makes me to put static_assert in the test. (not a big issue though)
philnik added inline comments.Jul 5 2022, 3:42 AM
libcxx/test/support/test_proxy.h
42 ↗(On Diff #442155)

I don't think you have any other option, since there are no standard copy-qualifier type traits and we haven't implemented any for the tests.
Just as a note: this isn't just a theoretical exercise. Microsoft uses the libc++ tests on their own STL implementation (i.e. to detect implementation divergence).

134 ↗(On Diff #442155)

I'm not sure if it should be done recursively. I think we had a discussion on this at some point, but I don't remember the outcome. Looking at the test_iterators it looks like we don't want to call base recursively. I think @ldionne should know that.

266 ↗(On Diff #442155)

Just to make sure: I'm not saying that the test is a bad idea. I just wanted to say that I would consider the test optional.

Since you bring it up: Maybe this should just be in test_iterators.h. I can't really see a case where you'd want this but not the other test iterators and you probably want this in most tests where you want the test iterators (at least going forward).

huixie90 added inline comments.Jul 5 2022, 5:18 AM
libcxx/test/support/test_proxy.h
42 ↗(On Diff #442155)

Thanks for explaining. I will try to find a different approach since the implementation of copy_cvref_t seems a lot of code. One way I can think of is that instead of having this one getData template function, I can have 4 overloads, that takes &, const&, &&, const&&, so that I can specify the cvref manually, which should be less code than copy then implementation. What do you htink

134 ↗(On Diff #442155)

Hmm. I am not so familiar with the design, the reason I call the base of the base is that without doing it, this doesn't work

ProxyIterator<cpp20_input_iterator> == sentinel_wrapper<ProxyIterator<cpp20_input_iterator>>

The reason is, sentinel_wrapper stores the base of the iterator on construction, and compare it with the base of the iterator to be compared.

In this case if the base of the ProxyIterator is cpp20_input_iterator, the above == doesn't work because cpp20_input_iterator cannot be compared with itself.

So I one step further to get the base of the base. I think the major difference is that when we use ProxyIterator, we will have one more level nested iterators than using other test iterators

What do you think?

266 ↗(On Diff #442155)

I will try to move the code into test_iterator

huixie90 updated this revision to Diff 442312.Jul 5 2022, 8:11 AM
huixie90 marked 4 inline comments as done.

addressed review comments

philnik accepted this revision as: philnik.Jul 5 2022, 8:24 AM

LGTM % nits.

libcxx/test/support/test_iterators.h
22–26

I don't think we need to guard the includes here. If you want to avoid depending on transitive includes we should probably add a modulemap. I'll make a PR for that. Sounds like a good idea either way.

816–822

Why aren't these member functions?

huixie90 updated this revision to Diff 442330.Jul 5 2022, 9:26 AM

Disable wsign-conversion for one test for now

huixie90 updated this revision to Diff 442336.Jul 5 2022, 9:37 AM
huixie90 marked 3 inline comments as done.

address review comments

huixie90 updated this revision to Diff 442374.Jul 5 2022, 12:38 PM

fix CI failure

huixie90 updated this revision to Diff 442381.Jul 5 2022, 1:08 PM

add == and <=> for Proxy (convinient for sort algorithms)

var-const requested changes to this revision.Jul 5 2022, 5:25 PM

This is very cool, thank you! I'd like to discuss (and potentially change) how tests are structured before approving.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
130

Rather than adding these checks to every test file, would it be possible to create a dedicated test file, similar to ranges_robust_against_copying_* files? Happy to discuss here or on Discord if you'd like.

libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp
23–24

Can you please provide more context here? It doesn't seem like either <chrono> or <atomic> (or <ranges>) are included here, and there don't seem to be any changes in includes in any of the standard headers, so why is this an issue now?

(Also: there's an extra 1 character in ADDITIONAL and a _TODO suffix which should probably be removed)

libcxx/test/support/test_iterators.h
802

Can you expand the comment to explain the intention here?

840

This is to make ProxyIterator, not Proxy, indirectly writable, right? If so, can you please make it clear in the comment?

860

These properties (std::indireclty_readable, std::indirectly_writable, anything else that's crucial for these classes to function correctly) should be static_asserted. You can add static assertions right after the class definitions.

875

Nit: s/dereference/dereferenced/.

878

Nit: s/e.g If/E.g. if/.

889

Add line break to place struct on a different line.

920

Nit: move requires to a different line?

1082

Ultranit: opening brace normally goes to the preceding line.

This revision now requires changes to proceed.Jul 5 2022, 5:25 PM
huixie90 added inline comments.Jul 5 2022, 11:35 PM
libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
130

I also like the idea of a single file, but it is slightly trickier than ranges_robust_against_copying_*. The ranges_robust_against_copying_* test doesn't check runtime behavior. I believe for these tests, we do want to check runtime behavior. For example, for the sorting tests, we do like to check the ranges are sorted. (e.g a triple-move implementation of swap could end up with elements overwritten, and only iter_swap can correctly swap the elements).

But we definitely "can" add a "ranges_robust_against_" test just to test it compiles for all algorithms for proxy iterator. and for individual algorithms that we know they are definitely moving/swapping elements, we can add additional tests to check that the behaviour is correct. what do you think

libcxx/test/support/test_proxy.h
161–165 ↗(On Diff #442155)

I couldn't because it has base class which don't have == defined. I think it is more effort to define the base class's operator== then just spell it out here

huixie90 updated this revision to Diff 442463.Jul 6 2022, 1:31 AM
huixie90 marked 9 inline comments as done.

address review feedback

huixie90 updated this revision to Diff 442514.Jul 6 2022, 4:38 AM

CI failure on mac clang

var-const accepted this revision as: var-const.Jul 6 2022, 3:01 PM
var-const added inline comments.
libcxx/test/support/test_iterators.h
802

Thanks! The class doesn't have special handling of swap to make sure there's a compilation error if the underlying algorithm uses plain swap, right? If yes, can you also add that to the comment -- perhaps something like "Note that unlike tuple, this class deliberately doesn't have special handling of swap to cause a compilation error if it's used in an algorithm that relies on plain swap instead of ranges::iter_swap".

huixie90 updated this revision to Diff 442698.Jul 6 2022, 3:17 PM
huixie90 marked an inline comment as done.

improve comments

philnik accepted this revision.Jul 7 2022, 3:02 PM
This revision is now accepted and ready to land.Jul 7 2022, 3:02 PM