[libcxx-commits] [PATCH] D117369: [libcxx][test] `unordered_meow` iterators are not portably non-bidi

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/test/std/containers/unord/unord.multimap/iterator_concept_conformance.compile.pass.cpp:66-67
 static_assert(!std::sized_sentinel_for<const_local_iterator, const_local_iterator>);
+static_assert(std::indirectly_movable<const_local_iterator, std::pair<int, int>*>);
+static_assert(!std::indirectly_movable_storable<const_local_iterator, std::pair<int, int>*>);
 static_assert(!std::indirectly_swappable<const_local_iterator, const_local_iterator>);
----------------
Quuxplusone wrote:
> CaseyCarter wrote:
> > Quuxplusone wrote:
> > > Pre-existing: I suspect that the intent was to use `value_type*` here, not `std::pair<int, int>*` (and likewise `value_type*` in place of `int*` in the unordered_set tests). No action required, but if you //want// to fix it, go for it. (I don't think the change should affect the results of these static_asserts.)
> > `value_type` is `pair<const int, int>`. `*declval<pair<const int, int>*>() = ranges::iter_move(some_iterator)` is never going to be a valid expression, so all of these `indirectly_movable{_storable,}` tests would yield false. That said, I'm not sure what value these provide - it may make sense to remove them in a followup.
> Ah, `indirectly_movable_storable` (https://en.cppreference.com/w/cpp/iterator/indirectly_movable_storable) is much less sane than I had assumed. ;) I had pictured basically what you might call "`indirectly_assignable_from`."
> 
> The operative element in reality is that it checks `std::assignable_from<std::iter_value_t<In>&, std::iter_rvalue_reference_t<In>>` for the //source// iterator. So `std::indirectly_movable_storable<unordered_map<K,V>::iterator, X> == false` for all X, because `std::assignable_from<unordered_map<K,V>::value_type, X> == false` for all X.
> So yeah, I see no value in these, and will submit a PR to remove them.
> 
> (This also suggests that the original STL chose `map::value_type` poorly — since `set<int>::value_type` is `int` not `const int`, then `map<int,int>::value_type` should have been `pair<int,int>` not `pair<const int,int>`. But that's water //long// under the bridge.)
The intuition is "`indirectly_copyable<I, O>` requires `*o = *i`. `indirectly_movable` requires `*o = ranges::iter_move(i)`. The `_storable` refinements require that transfer to be valid both directly, and indirectly through a variable of `I`'s `value_type` via both construction and assignment.

And yes, map::value_type would be different today. There wasn't much they could have done differently in 1998 if they wanted to forbid changing map keys via iterator references - remember that Cpp17ForwardIterator must have a reference type of value_type& or const value_type&.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117369