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

Code before:
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Compiler.h"
#include "toolchain/base/mem_usage.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

#ifndef CARBON_POISON_VALUE_STORES
#define CARBON_POISON_VALUE_STORES 0
#elif !LLVM_ADDRESS_SANITIZER_BUILD
#error "CARBON_POISON_VALUE_STORES requires address sanitizer"
#endif

// A simple wrapper for accumulating values, providing IDs to later retrieve the
// value. This does not do deduplication.
//
// IdT::ValueType must represent the type being indexed.
template <typename IdT>
class ValueStore
    : public std::conditional<
          std::is_base_of_v<Printable<typename IdT::ValueType>,
                            typename IdT::ValueType>,
          Yaml::Printable<ValueStore<IdT>>, Internal::ValueStoreNotPrintable> {
 public:
  using ValueType = typename IdT::ValueType;

  // 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&>;

  ValueStore() = default;
  ValueStore(ValueStore&& other) noexcept
      : values_((other.UnpoisonAll(), std::move(other.values_)))
#if CARBON_POISON_VALUE_STORES
        ,
        all_poisoned_(false)

Review comment (bug):
I'd add some documentation here. The bazel flag seems awfully distant from this code, so some cross-connecting docs / TODOs seem helpful.

```suggestion
// Setup our compile time condition controlling poisoning of value stores. This is set to one by the Bazel flag `--features=poison_value_stores`.
//
// TODO: Eventually, this will always enabled when ASan is enabled, but we can't do that until we clean up all of the latent bugs.
#ifndef CARBON_POISON_VALUE_STORES
#define CARBON_POISON_VALUE_STORES 0
#elif !LLVM_ADDRESS_SANITIZER_BUILD
#error "CARBON_POISON_VALUE_STORES requires address sanitizer"
#endif
```

Code after:
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/Support/Compiler.h"
#include "toolchain/base/mem_usage.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

// Setup our compile time condition controlling poisoning of value stores. This
// is set to one by the Bazel flag `--features=poison_value_stores`.
//
// TODO: Eventually, this will always enabled when ASan is enabled, but we can't
// do that until we clean up all of the latent bugs.
#ifndef CARBON_POISON_VALUE_STORES
#define CARBON_POISON_VALUE_STORES 0
#elif !LLVM_ADDRESS_SANITIZER_BUILD
#error "CARBON_POISON_VALUE_STORES requires address sanitizer"
#endif

// A simple wrapper for accumulating values, providing IDs to later retrieve the
// value. This does not do deduplication.
//
// IdT::ValueType must represent the type being indexed.
template <typename IdT>
class ValueStore
    : public std::conditional<
          std::is_base_of_v<Printable<typename IdT::ValueType>,
                            typename IdT::ValueType>,
          Yaml::Printable<ValueStore<IdT>>, Internal::ValueStoreNotPrintable> {
 public:
  using ValueType = typename IdT::ValueType;

  // 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&>;

  ValueStore() = default;