Repository: carbon-language/carbon-lang
File: toolchain/base/value_store.h (C/C++)

Code before:
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Compiler.h"
#include "toolchain/base/mem_usage.h"
#include "toolchain/base/value_store_chunk.h"
#include "toolchain/base/yaml.h"

namespace Carbon {

namespace Internal {

// Used as a parent class for non-printable types. This is just for
// std::conditional, not as an API.
class ValueStoreNotPrintable {};

}  // namespace Internal

template <class IdT>
class ValueStoreRange;

// Common calculation for ValueStore types.
template <typename IdT, typename ValueT = IdT::ValueType,
          typename KeyT = ValueT>
class ValueStoreTypes {
 public:
  using ValueType = std::decay_t<ValueT>;
  using KeyType = std::decay_t<KeyT>;

  // Typically we want to use `ValueType&` and `const ValueType& to avoid
  // copies, but when the value type is a `StringRef`, we assume external
  // storage for the string data and both our value type and ref type will be
  // `StringRef`. This will preclude mutation of the string data.
  using RefType = std::conditional_t<std::same_as<llvm::StringRef, ValueType>,
                                     llvm::StringRef, ValueType&>;
  using ConstRefType =
      std::conditional_t<std::same_as<llvm::StringRef, ValueType>,
                         llvm::StringRef, const ValueType&>;
};

// If `IdT` provides a distinct `IdT::KeyType`, default to that for the key
// type.
template <typename IdT>
  requires(!std::same_as<typename IdT::ValueType, typename IdT::KeyType>)
class ValueStoreTypes<IdT>
    : public ValueStoreTypes<IdT, typename IdT::ValueType,
                             typename IdT::KeyType> {};

// A simple wrapper for accumulating values, providing IDs to later retrieve the

Review comment (suggestion):
`ValueStoreTypes` had been intended for things used by multiple value store types; shouldn't the key type be specific to `CanonicalValueStore`? Note mostly I'm wondering if it'd be a little easier to understand when the type is used if it weren't provided for value stores that lack a key.

Not sure if this implies it'd be easier to pass in `KeyT` to the `CanonicalValueStore` template instead of having it inferred -- I'm fine if you don't want to deal with that here, it's something I'm more generally considering if we should just do things like `using FooStore = ValueStore<IdT, ValueT>` instead of `using ValueType = ...` and inferring based on that.

(maybe a TODO to remove from KeyT from ValueStoreTypes, if there's nowhere else to use it?)

Code after:
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Compiler.h"
#include "toolchain/base/mem_usage.h"
#include "toolchain/base/value_store_chunk.h"
#include "toolchain/base/yaml.h"

namespace Carbon {

namespace Internal {

// Used as a parent class for non-printable types. This is just for
// std::conditional, not as an API.
class ValueStoreNotPrintable {};

}  // namespace Internal

template <class IdT>
class ValueStoreRange;

// Common calculation for ValueStore types.
template <typename IdT, typename ValueT = IdT::ValueType,
          typename KeyT = ValueT>
class ValueStoreTypes {
 public:
  using ValueType = std::decay_t<ValueT>;

  // TODO: Would be a bit cleaner to not have this here as it's only meaningful
  // to the `CanonicalValueStore`, not to other `ValueStore`s. Planned to fix
  // with a larger refactoring.
  using KeyType = std::decay_t<KeyT>;

  // Typically we want to use `ValueType&` and `const ValueType& to avoid
  // copies, but when the value type is a `StringRef`, we assume external
  // storage for the string data and both our value type and ref type will be
  // `StringRef`. This will preclude mutation of the string data.
  using RefType = std::conditional_t<std::same_as<llvm::StringRef, ValueType>,
                                     llvm::StringRef, ValueType&>;
  using ConstRefType =
      std::conditional_t<std::same_as<llvm::StringRef, ValueType>,
                         llvm::StringRef, const ValueType&>;
};

// If `IdT` provides a distinct `IdT::KeyType`, default to that for the key
// type.
template <typename IdT>
  requires(!std::same_as<typename IdT::ValueType, typename IdT::KeyType>)
class ValueStoreTypes<IdT>