teslamotors / fixed-containers

C++ Fixed Containers
MIT License
377 stars 33 forks source link

[FixedMap/EnumMap] Fix dangling references by changing iter return type #45

Closed alexkaratarakis closed 1 year ago

alexkaratarakis commented 1 year ago

Currently, FixedMap and EnumMap return a PairView<const K, const V>& (notably, a reference in contrast to a value) in order to be able to support the same syntax that std::map does when iterating, i.e.:

for (auto&& pair : map) {} // a
for (const auto& pair : map) {} // b
for (auto& pair : map) {} // c

However, this makes it possible for dangling references to occur.

decltype(m)::reference ref = *m.begin(); // Dangling // 1
auto ref = *m.begin();  // Fine // 2
auto& ref = *m.begin();  // Dangling // 3

(all 3 are fine for std::map).

Problems: 1) Cannot store a std::pair<const K, V> because it is not assignable and not trivially copyable, so it is not possible to give a reference to it. Also, for EnumMap the key is not even stored, so there is no pair at all. further explanation 2) To synthesize a reference (as opposed to a value) the iterator is taxed with keeping the storage alive, so if it goes out of scope, then the reference is dangling, which is what happens with 1 and 3. 2 does a copy, so it is safe.

To address the issue, return PairView<const K, const V> by value (not a reference). This fixes dangling issues as demo-ed in the unit tests. This makes it so syntax c is an error, and syntax b is a warning (which we can disable). Syntax a works as always source.

This also makes it unnecessary to use PairView, and allows switching to std::pair which removes the first()/second() API discrepancy, outlined here. This will be done in a subsequent change to std::pair<const K&, V&>, which is also the type used by the upcoming (C++23) std::flat_map

Relevant discussion: https://github.com/teslamotors/fixed-containers/pull/34 https://github.com/teslamotors/fixed-containers/pull/38