From 55e567537301351ccc85cccc5cb895dc7bc64409 Mon Sep 17 00:00:00 2001 From: Geoff Romer Date: Thu, 13 Nov 2025 15:13:32 -0800 Subject: [PATCH] Clarify const semantics of `Set` and `Map` (#6351) Also add missing `const` to `ForEach` on `Set` and `SetView`. This is an alternative to #6347, depending on the const semantics we want here. --- common/map.h | 33 +++++++++++++++++++++------------ common/set.h | 37 ++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/common/map.h b/common/map.h index 1de36e376ebd..8fbdb6c02b3d 100644 --- a/common/map.h +++ b/common/map.h @@ -33,16 +33,16 @@ class Map; // structure. // // This should always be preferred to a `const`-ref parameter for the `MapBase` -// or `Map` type as it provides more flexibility and a cleaner API. -// -// Note that while this type is a read-only view, that applies to the underlying -// *map* data structure, not the individual entries stored within it. Those can -// be mutated freely as long as both the hashes and equality of the keys are -// preserved. If we applied a deep-`const` design here, it would prevent using -// this type in many useful situations where the elements are mutated but the -// associative container is not. A view of immutable data can always be obtained -// by using `MapView`, and we enable conversions to more-const -// views. This mirrors the semantics of views like `std::span`. +// or `Map` type as it provides more flexibility and a cleaner API. By default a +// `MapView` provides no more immutability than a `const Map`: elements can't be +// added or removed, but both keys and values can be mutated, and the user is +// responsible for avoiding mutations that affect the key's hash value or +// equality comparison. However, the key and value types can be +// `const`-qualified to prevent those mutations. For example, a `MapView` +// can be converted to `MapView` to prevent mutating the keys. As +// with any other view type, `const` on the `MapView` itself is "shallow": it +// prevents rebinding the `MapView` to a different underlying map, but doesn't +// affect mutability of the underlying map. // // A specific `KeyContextT` type can optionally be provided to configure how // keys will be hashed and compared. The default is `DefaultKeyContext` which is @@ -142,6 +142,14 @@ class MapView // A base class for a `Map` type that remains mutable while type-erasing the // `SmallSize` (SSO) template parameter. // +// Note that `MapBase` has "shallow" const semantics: a `const MapBase&` +// can't be used to mutate the map data structure itself (e.g. by changing the +// number of elements), but it can be used to mutate the keys and values it +// contains. The user is responsible for avoiding mutations that would change +// the hash value or equality of a key. A `MapView` with const-qualified key and +// value types can be used to provide read-only access to the elements of a +// `MapBase`. +// // A pointer or reference to this type is the preferred way to pass a mutable // handle to a `Map` type across API boundaries as it avoids encoding specific // SSO sizing information while providing a near-complete mutable API. @@ -365,8 +373,9 @@ class MapBase : protected RawHashtable::BaseImpl class Map : public RawHashtable::TableImpl< diff --git a/common/set.h b/common/set.h index 5562d396b2a5..29b462614244 100644 --- a/common/set.h +++ b/common/set.h @@ -30,17 +30,15 @@ class Set; // structure. // // This should always be preferred to a `const`-ref parameter for the `SetBase` -// or `Set` type as it provides more flexibility and a cleaner API. -// -// Note that while this type is a read-only view, that applies to the underlying -// *set* data structure, not the individual entries stored within it. Those can -// be mutated freely as long as both the hashes and equality of the keys are -// preserved. If we applied a deep-`const` design here, it would prevent using -// this type in situations where the keys carry state (unhashed and not part of -// equality) that is mutated while the associative container is not. A view of -// immutable data can always be obtained by using `SetView`, and we -// enable conversions to more-const views. This mirrors the semantics of views -// like `std::span`. +// or `Set` type as it provides more flexibility and a cleaner API. By default +// a `SetView` provides no more immutability than a `const Set`: elements +// can't be added or removed, but they can be mutated, and the user is +// responsible for avoiding mutations that affect the hash value or equality +// comparison. However, a `SetView` can be converted to a `SetView`, +// which prevents mutating the elements. As with any other view type, `const` +// on the `SetView` itself is "shallow": it prevents rebinding the `SetView` to +// a different underlying set, but doesn't affect mutability of the underlying +// set. // // A specific `KeyContextT` type can optionally be provided to configure how // keys will be hashed and compared. The default is `DefaultKeyContext` which is @@ -95,7 +93,7 @@ class SetView : RawHashtable::ViewImpl { // Run the provided callback for every key in the set. template - auto ForEach(CallbackT callback) -> void + auto ForEach(CallbackT callback) const -> void requires(std::invocable); // This routine is relatively inefficient and only intended for use in @@ -123,6 +121,13 @@ class SetView : RawHashtable::ViewImpl { // A base class for a `Set` type that remains mutable while type-erasing the // `SmallSize` (SSO) template parameter. // +// Note that `SetBase` has "shallow" const semantics: a `const SetBase&` +// can't be used to mutate the set data structure itself (e.g. by changing +// the number of elements), but it can be used to mutate the `T` elements it +// contains. The user is responsible for avoiding mutations that would change +// the hash value or equality of an element. A `SetView` can be used +// to provide read-only access to the elements of a `SetBase`. +// // A pointer or reference to this type is the preferred way to pass a mutable // handle to a `Set` type across API boundaries as it avoids encoding specific // SSO sizing information while providing a near-complete mutable API. @@ -186,7 +191,7 @@ class SetBase // Convenience forwarder to the view type. template - auto ForEach(CallbackT callback) -> void + auto ForEach(CallbackT callback) const -> void requires(std::invocable) { return ViewT(*this).ForEach(callback); @@ -283,7 +288,8 @@ class SetBase // allocations the performance of hashtable routines may be unacceptably bad and // another data structure or key design is likely preferable. // -// Note that this type should typically not appear on API boundaries; either +// Note that `Set`, like `SetBase`, has "shallow" const semantics. Note also +// that this type should typically not appear on API boundaries; either // `SetBase` or `SetView` should be used instead. template @@ -328,7 +334,8 @@ auto SetView::Lookup(LookupKeyT lookup_key, template template -auto SetView::ForEach(CallbackT callback) -> void +auto SetView::ForEach(CallbackT callback) const + -> void requires(std::invocable) { this->ForEachEntry([callback](EntryT& entry) { callback(entry.key()); },