This addresses the usage of operator& in <unordered_map>.
(Note there are still more headers with the same issue.)
Details
Details
- Reviewers
• Quuxplusone ldionne - Group Reviewers
Restricted Project - Commits
- rGd6e2c95d2252: [libc++] Use addressof in unordered_map.
rGcab961693802: [libc++] Use addressof in unordered_map.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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 | |
| 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. 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. | |
| 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?
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. | |
| 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. | |
| libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp | ||
|---|---|---|
| 42–43 | Your new test looks good to me! | |
Comment Actions
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 | ||
Comment Actions
Basically I add tests for things that aren't ADL-hijacking proof. So basically they are regression tests.
Comment Actions
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.
@Quuxplusone Just a pointer to the code I refer to later.