This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix incorrect iterator type in vector container test.
ClosedPublic

Authored by amakc11 on Nov 21 2018, 10:41 AM.

Details

Reviewers
EricWF
ldionne
Summary

The iterator types for different specializations of containers with the same element type but different allocators are not required to be convertible. This patch makes the test to take the iterator type from the same container specialization as the created container.

Diff Detail

Repository
rCXX libc++

Event Timeline

amakc11 created this revision.Nov 21 2018, 10:41 AM
ldionne requested changes to this revision.Nov 21 2018, 10:52 AM
ldionne added inline comments.
test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
98

Please introduce a typedef to avoid repeating std::vector<int, limited_allocator<int, 308> >.

This revision now requires changes to proceed.Nov 21 2018, 10:52 AM

My intention was to be consistent with the #if TEST_STD_VER >= 11 segment of the test, which does not introduce any typedef in the similar context. Should I change both?

My intention was to be consistent with the #if TEST_STD_VER >= 11 segment of the test, which does not introduce any typedef in the similar context. Should I change both?

Ah, I missed that. Please make the change throughout. Otherwise, one can wonder whether these tests are meant to check for SCARY iterators, which they aren't.

amakc11 updated this revision to Diff 174955.Nov 21 2018, 11:42 AM

typedef added throughout the test to avoid confusion whether the implemented test actions are meant to check for SCARY iterators.

BTW, which tests in the test suite are meant to check for SCARY iterators?

BTW, which tests in the test suite are meant to check for SCARY iterators?

The only ones I could find with a quick grep are:

libcxx/test/std/containers/unord/unord.multimap/scary.pass.cpp
libcxx/test/std/containers/unord/unord.multiset/scary.pass.cpp
libcxx/test/std/containers/associative/multimap/scary.pass.cpp
libcxx/test/std/containers/associative/multiset/scary.pass.cpp

There doesn't seem to be such tests for all containers. However, I don't this SCARY iterators are actually required by the standard (am I wrong?).

ldionne requested changes to this revision.Nov 21 2018, 12:00 PM
ldionne added inline comments.
test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
98

You need spaces between consecutive >>'s, otherwise the test won't pass in C++03 mode.

This revision now requires changes to proceed.Nov 21 2018, 12:00 PM
ldionne added inline comments.Nov 21 2018, 12:01 PM
test/std/containers/sequences/vector/vector.modifiers/insert_iter_iter_iter.pass.cpp
98

Also, we usually use names like V instead of test_vector_type, but it doesn't really matter. Just FYI.

However, I don't this SCARY iterators are actually required by the standard (am I wrong?).

You are right: SCARY iterators are permitted but not required by the standard. Thanks for the tests list.

amakc11 updated this revision to Diff 174965.Nov 21 2018, 12:23 PM

Missing space restored. Type name changed according to the coding standard (sorry, I prefer readable identifiers by default).

ldionne accepted this revision.Nov 21 2018, 12:56 PM

LGTM. Committed as r347423. Thanks for the patch!

This revision is now accepted and ready to land.Nov 21 2018, 12:56 PM
ldionne closed this revision.Nov 21 2018, 12:56 PM