This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use addressof in unordered_map.
ClosedPublic

Authored by Mordante on Jan 15 2022, 4:01 AM.

Details

Summary

This addresses the usage of operator& in <unordered_map>.

(Note there are still more headers with the same issue.)

Diff Detail

Event Timeline

Mordante requested review of this revision.Jan 15 2022, 4:01 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2022, 4:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 400275.Jan 15 2022, 4:38 AM

Fix CI, but disabling tests for older language standards.

Quuxplusone requested changes to this revision.Jan 15 2022, 6:31 AM
Quuxplusone added a subscriber: Quuxplusone.

Obviously looks basically fine, but I have some significant test comments, I think.

libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
10

I think unordered_map is supported (by libc++) in C++03 mode. You might need to UNSUPPORTED: c++03 in a small number of tests that test C++11 verbs like emplace, but not in tests like this one.

18

s/tje /the/

42–43

Lines 41 and 42 are testing the same type: const_iterator (line 41 because it's explicitly specified, line 42 because it's deduced). If this test is trying to test the const-qualified begin() method, then you need this:

test(m.begin());
test(static_cast<const decltype(m)&>(m).begin());
test(m.cbegin());

test(m.begin(0));
test(static_cast<const decltype(m)&>(m).begin(0));
test(m.cbegin(0));

But the name of the test implies that begin is irrelevant and it's just trying to test iterator operations, i.e., you can just do

test<decltype(m)::iterator>();
test<decltype(m)::const_iterator>();
test<decltype(m)::local_iterator>();
test<decltype(m)::const_local_iterator>();

and define test as just

template<class It>
void test() {
    It it;
    It jt = it;
    jt = it;
    jt = It(std::move(it));
}
libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_move.addressof.compile.pass.cpp
25

move-assigned

This revision now requires changes to proceed.Jan 15 2022, 6:31 AM
Mordante marked 4 inline comments as done.Jan 15 2022, 11:01 AM
Mordante added inline comments.
libcxx/include/__hash_table
666

@Quuxplusone Just a pointer to the code I refer to later.

libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
42–43

It's not testing the same thing.
test(m.begin()); -> __hash_map_iterator(const __hash_map_iterator&)
test(m.cbegin()); -> __hash_map_const_iterator(const __hash_map_const_iterator&)
test<C>(m.begin()); -> __hash_map_const_iterator(const __hash_map_iterator&)

This can easily be tested by commenting out the constructor in include/unordered_map:970, but the real issue is in include/__hash_table:666. There I needed to make a change to guard against ADL-hijacking. So the additional argument is not for deducing, but to call the converting constructor.

I thought this was clear, but your remarks proofs differently. I'll add a comment.

The tests are only about type checking, all tests are compile-time tests and not run-time tests. I prefer to keep the code as is, since all similar tests are in a similar fashion.

Mordante updated this revision to Diff 400309.Jan 15 2022, 11:04 AM
Mordante marked an inline comment as done.

Addresses review comments.

libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
42–43

Well, at least do you agree that since ToIterator is invariably a synonym for FromIterator, you can eliminate it?

The tests are only about type checking, all tests are compile-time tests and not run-time tests.

Agreed; I wasn't proposing to make this test a runtime test. I was just saying that it would be simpler to write test as

template<class It>  // `It` corresponds to your `FromIterator`/`ToIterator` parameter
void test() {
    It it;
    It jt = it;  // test the copy ctor
    jt = it;  // test copy assignment
    jt = It(std::move(it));  // test the move ctor and then test move assignment
}

but yes, we're still just testing that these four lines compile; we're not testing their runtime behavior at all.

If you want to test the converting ctor from iterator to const_iterator, you'll have to do something different from either what's-here-now or what-I-proposed.

Mordante updated this revision to Diff 401326.Jan 19 2022, 10:27 AM

Addresses review comments.

Mordante marked an inline comment as done.Jan 19 2022, 10:29 AM
Mordante added inline comments.
libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
42–43

No there were other combinations tested. I've rewritten the test to make it clear what's tested.
Somewhat based on your tests, but I dislike the last line where two tests are hidden on one line.
When a tests fails I like to just look at the offending line.

Quuxplusone added inline comments.
libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
42–43

Your new test looks good to me!

What is the reasoning for which member functions should be checked?

libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp
31
ldionne accepted this revision.Jan 20 2022, 8:22 AM
This revision is now accepted and ready to land.Jan 20 2022, 8:22 AM
Mordante updated this revision to Diff 401665.Jan 20 2022, 9:13 AM
Mordante marked an inline comment as done.

Fix CI.

Mordante marked 2 inline comments as done.Jan 20 2022, 9:16 AM

What is the reasoning for which member functions should be checked?

Basically I add tests for things that aren't ADL-hijacking proof. So basically they are regression tests.

Shouldn't there also be a test for insert_or_assign then?

Shouldn't there also be a test for insert_or_assign then?

They didn't fail for me, so I haven't been able get test failure with them. So I prefer the whack-a-mole approach for this. Writing tests for all possible cases seems a lot of work with little benefit. But feel free to add additional tests.

This revision was automatically updated to reflect the committed changes.
Mordante reopened this revision.Jan 21 2022, 9:10 AM
This revision is now accepted and ready to land.Jan 21 2022, 9:10 AM
Mordante updated this revision to Diff 402010.Jan 21 2022, 9:11 AM

Give it another CI run to make sure I applied the proper fixes.

This revision was automatically updated to reflect the committed changes.