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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__hash_table:666
 #if _LIBCPP_DEBUG_LEVEL == 2
-        __get_db()->__iterator_copy(this, &__x);
+        __get_db()->__iterator_copy(this, _VSTD::addressof(__x));
 #endif
----------------
@Quuxplusone Just a pointer to the code I refer to later.


================
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());
+
----------------
Quuxplusone wrote:
> 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));
> }
> ```
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. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117393