Consistently DCHECK the index >= 0 in value store getters

toolchain/base/canonical_value_store.h

@@ -69,9 +69,11 @@ class CanonicalValueStore {
     mem_usage.Add(MemUsage::ConcatLabel(label, "set_"), bytes, bytes);
   }
 
-  auto GetRawIndex(IdT id) const -> int32_t { return values_.GetRawIndex(id); }
-
   auto GetIdTag() const -> IdTag { return values_.GetIdTag(); }
+  auto GetRawIndex(IdT id) const -> int32_t { return values_.GetRawIndex(id); }
+  auto TryGetRawIndex(IdT id) const -> int32_t {
+    return values_.TryGetRawIndex(id);
+  }
 
  private:
   class KeyContext;

toolchain/base/relational_value_store.h

@@ -26,7 +26,7 @@ namespace Carbon {
 //
 // When adding to the store, the user provides the related
 // `RelatedStoreT::IdType` along with the value being stored, and gets back the
-// ID of the value in the store.
+// ID of the value in the store. The ids share an IdTag with the related store.
 //
 // This store requires more storage space than normal ValueStore does, as it
 // requires storing a bit for presence of each `RelatedStore::IdType`. And it
@@ -45,34 +45,36 @@ class RelationalValueStore {
   // Given the related ID and a value, stores the value and returns a mapped ID
   // to reference it in the store.
   auto Add(RelatedIdType related_id, ValueType value) -> IdT {
-    auto related_index = related_store_->GetRawIndex(related_id);
+    auto related_index = related_store_->TryGetRawIndex(related_id);
+    CARBON_DCHECK(related_index >= 0, "{0}", related_id);
     if (static_cast<size_t>(related_index) >= values_.size()) {
       values_.resize(related_index + 1);
     }
     auto& opt = values_[related_index];
     CARBON_CHECK(!opt.has_value(),
                  "Add with `related_id` that was already added to the store");
     opt.emplace(std::move(value));
-    return IdT(related_store_->GetIdTag().Apply(related_index));
+    return IdT(related_id.index);
   }
 
   // Returns the ID of a value in the store if the `related_id` was previously
   // used to add a value to the store, or None.
   auto TryGetId(RelatedIdType related_id) const -> IdT {
-    auto related_index = related_store_->GetRawIndex(related_id);
+    auto related_index = related_store_->TryGetRawIndex(related_id);
+    CARBON_DCHECK(related_index >= 0, "{0}", related_id);
     if (static_cast<size_t>(related_index) >= values_.size()) {
       return IdT::None;
     }
     auto& opt = values_[related_index];
     if (!opt.has_value()) {
       return IdT::None;
     }
-    return IdT(related_store_->GetIdTag().Apply(related_index));
+    return IdT(related_id.index);
   }
 
   // Returns a value for an ID.
   auto Get(IdT id) const -> ConstRefType {
-    auto index = related_store_->GetIdTag().Remove(id.index);
+    auto index = related_store_->TryGetRawIndex(RelatedIdType(id.index));
     CARBON_DCHECK(index >= 0, "{0}", id);
     return *values_[index];
   }

toolchain/base/value_store.h

@@ -209,15 +209,17 @@ class ValueStore
 
   // Returns a mutable value for an ID.
   auto Get(IdType id) -> RefType {
-    CARBON_DCHECK(id.index >= 0, "{0}", id);
-    auto [chunk_index, pos] = IdToChunkIndices(id);
+    auto raw_index = TryGetRawIndex(id);
+    CARBON_DCHECK(raw_index >= 0, "{0}", id);
+    auto [chunk_index, pos] = RawIndexToChunkIndices(raw_index);
     return chunks_[chunk_index].Get(pos);
   }
 
   // Returns the value for an ID.
   auto Get(IdType id) const -> ConstRefType {
-    CARBON_DCHECK(id.index >= 0, "{0}", id);
-    auto [chunk_index, pos] = IdToChunkIndices(id);
+    auto raw_index = TryGetRawIndex(id);
+    CARBON_DCHECK(raw_index >= 0, "{0}", id);
+    auto [chunk_index, pos] = RawIndexToChunkIndices(raw_index);
     return chunks_[chunk_index].Get(pos);
   }
 
@@ -310,9 +312,18 @@ class ValueStore
   }
 
   auto GetIdTag() const -> IdTag { return tag_; }
+  // Removes the IdTag from the `id` and returns the absolute index into the
+  // store. Not valid to call for negative values, which are not part of the
+  // store.
   auto GetRawIndex(IdT id) const -> int32_t {
+    auto index = TryGetRawIndex(id);
+    CARBON_DCHECK(index >= 0);
+    return index;
+  }
+  // Like GetRawIndex() but accepts negative values like `None` and passes them
+  // through, even though they would not be a valid index into an array.
+  auto TryGetRawIndex(IdT id) const -> int32_t {
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", index);
 #ifndef NDEBUG
     if (index >= size_) {
       // Attempt to decompose id.index to include extra detail in the check
@@ -470,12 +481,6 @@ class ValueStore
     return {chunk, pos};
   }
 
-  // Converts an id into an index into the set of chunks, and an offset into
-  // that specific chunk.
-  auto IdToChunkIndices(IdType id) const -> std::pair<int32_t, int32_t> {
-    return RawIndexToChunkIndices(GetRawIndex(id));
-  }
-
   // Number of elements added to the store. The number should never exceed what
   // fits in an `int32_t`, which is checked in non-optimized builds in Add().
   int32_t size_ = 0;