Rework iterators to remove first()/second() api discrepancy

[alexkaratarakis]

Proposed change vs PairView

Pros:

• No API discrepancy!

Cons:

• const iterator (no underscore) doesn't propagate const. e.g. const std::pair<const int&, int&> allows mutation of .second. PairView was propagating const
• Iterators no longer std::is_trivially_copyable_v because of the assignability mechanisms on top of storing a pair of references likestd::pair<const int&, int&>

EDIT: const iterator is indeed unchanged (see comments below), so point is amended to:

    using M = fixed_containers::FixedMap<int, int, 42>;
    M::reference r = *it;
    const auto& jr = *jt;
     jr.second() = 1; // Disallowed with PairView

    using M = fixed_containers::FixedMap<int, int, 42>;
    M::reference r = *it;
    const auto& jr = *jt;
     jr.second = 1; // Now allowed

See also iterator implementation with ArrowProxy: https://github.com/teslamotors/fixed-containers/pull/39

[Quuxplusone]

const iterator (no underscore) doesn't propagate const. e.g. const std::pair<const int&, int&> allows mutation of .second. PairView was propagating const.

Isn't that what you want, though? The following should work, and IIUC does work, both before and after this patch. So I don't think this is a "con" of the new approach; I think it's a "no difference."

#ifdef OLD_WAY
 #define second second()
#endif

int main()
{
  using M = fixed_containers::FixedMap<int, int, 42>;
  M m = {{1,2}};

  M::iterator it = m.begin();
  const M::iterator jt = it;
  it->second = 1; // OK
  jt->second = 1; // OK

  M::reference r = *it;
  const M::reference jr = *jt;  // warns, but OK
  r.second = 1; // OK
  jr.second = 1; // OK
}

However, I suspect that this might be a con:

  M::reference dr = *m.begin();  // Compiles; but doesn't this dangle immediately?

If I'm right, this is a side-effect of #38's unusual choice to make M::reference an alias for std::pair<const K&, V&>& instead of just std::pair<const K&, V&> (as in #39).

Re #39's interaction with -Wrange-loop-bind-reference: Yeah, std::flat_map triggers the same warning (Godbolt). But, that warning is off-by-default precisely because it has false positives like this. So the user would have to be actively opting in to that warning, and using const auto& for-loops in concrete flat_map/FixedMap contexts. (In template contexts where you don't know what you're iterating over, you must use auto&& as far as I'm concerned.) So the answer is "don't do that."

Anyway, I haven't looked closely enough at #39 to say that it's 100% correct; and I haven't looked closely enough at #38 to really know exactly how it differs from #39; but I continue to think you're on top of the situation. :)

[alexkaratarakis]

const iterator is unchanged

Your example is correct. I was thinking-of/testing-with the type declaration const auto& (instead of const M::iterator), where the const-propagation is applied because it resolves to const PairView<const int, int>&

    using M = fixed_containers::FixedMap<int, int, 42>;
    M::reference r = *it;
    const auto& jr = *jt;
     jr.second() = 1; // Disallowed with const PairView<const int, int>&

    using M = fixed_containers::FixedMap<int, int, 42>;
    M::reference r = *it;
    const auto& jr = *jt;
     jr.second = 1; // Allowed with const std::pair<const int&, int&>&

M::reference dr = *m.begin(); // Compiles; but doesn't this dangle immediately?

You are right, this is dangling. I think this makes this approach non-viable :( Added demo where ASAN highlights the bug for the future.

-Wrange-loop-bind-reference

auto&& in templates

Also valid points :)

[alexkaratarakis]

Closing in favor of: https://github.com/teslamotors/fixed-containers/pull/45 https://github.com/teslamotors/fixed-containers/pull/48

[alexkaratarakis]

For future reference: Attempted to make this work by 1) changing the reference type declared to PairView//std::pair (instead of PairView&/std::pair&) and then applying the & on return, but this means the actual type returned is not quite the type reference says it is. 2) Using r-value overloads:

    constexpr reference operator*() const& noexcept { return reference_provider_.get(); }
    constexpr std::conditional_t<SAFE_LIFETIME, reference, value_type> operator*() && noexcept
    {
        return reference_provider_.get();
    }

but seems problematic. Going for std::pair<const K&, V&> (by value) and ArrowProxy instead