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

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

Quuxplusone accepted this revision as: Quuxplusone.
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:
> > 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.
Your new test looks good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117393