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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante 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());
+
----------------
Quuxplusone wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117393