[libcxx-commits] [PATCH] D117393: [libc++] Use addressof in unordered_map.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03
+
----------------
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.


================
Comment at: libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp:17
+
+// Validate the constructors of tje  (const)(_local)_iterator classes to be
+// properly guarded against ADL-hijacking operator&.
----------------
`s/tje /the/`


================
Comment at: libcxx/test/std/containers/unord/unord.map/iterator.operators.addressof.compile.pass.cpp:41-42
+  test(m.begin());
+  test<C>(m.begin());
+  test(m.cbegin());
+
----------------
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));
}
```


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_move.addressof.compile.pass.cpp:24
+
+// Validate whether the container can be copy-assigned with an ADL-hijacking operator&
+
----------------
move-assigned


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117393/new/

https://reviews.llvm.org/D117393