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

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

Quuxplusone added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117393