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

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

Quuxplusone 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>);
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117369