Add a function to translate a canonical value to key. (#6222)

This is to address concerns about the `==` and hash duplication.

Author: jonmeow

toolchain/base/canonical_value_store.h

@@ -22,7 +22,13 @@ namespace Carbon {
 // `KeyT` can optionally be different from `ValueT`, and if so is used for the
 // argument to `Lookup`. It must be valid to use both `KeyT` and `ValueT` as
 // lookup types in the underlying `Set`.
-template <typename IdT, typename KeyT, typename ValueT = KeyT>
+template <typename IdT, typename KeyT, typename ValueT = KeyT,
+          // Parentheses around the lambda to help clang-format.
+          auto ValueToKeyFn =
+              ([](typename ValueStoreTypes<ValueT>::ConstRefType value) ->
+               typename ValueStoreTypes<ValueT>::ConstRefType {
+                 return value;
+               })>
 class CanonicalValueStore {
  public:
   using KeyType = std::remove_cvref_t<KeyT>;
@@ -75,37 +81,43 @@ class CanonicalValueStore {
   Set<IdT, /*SmallSize=*/0, KeyContext> set_;
 };
 
-template <typename IdT, typename KeyT, typename ValueT>
-class CanonicalValueStore<IdT, KeyT, ValueT>::KeyContext
+template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
+class CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::KeyContext
     : public TranslatingKeyContext<KeyContext> {
  public:
   explicit KeyContext(const ValueStore<IdT, ValueType>* values)
       : values_(values) {}
 
   // Note that it is safe to return a `const` reference here as the underlying
   // object's lifetime is provided by the `ValueStore`.
-  auto TranslateKey(IdT id) const -> ConstRefType { return values_->Get(id); }
+  auto TranslateKey(IdT id) const
+      -> llvm::function_traits<decltype(ValueToKeyFn)>::result_t {
+    return ValueToKeyFn(values_->Get(id));
+  }
 
  private:
   const ValueStore<IdT, ValueType>* values_;
 };
 
-template <typename IdT, typename KeyT, typename ValueT>
-auto CanonicalValueStore<IdT, KeyT, ValueT>::Add(ValueType value) -> IdT {
+template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
+auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Add(ValueType value)
+    -> IdT {
   auto make_key = [&] { return IdT(values_.Add(std::move(value))); };
-  return set_.Insert(value, make_key, KeyContext(&values_)).key();
+  return set_.Insert(ValueToKeyFn(value), make_key, KeyContext(&values_)).key();
 }
 
-template <typename IdT, typename KeyT, typename ValueT>
-auto CanonicalValueStore<IdT, KeyT, ValueT>::Lookup(KeyType key) const -> IdT {
+template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
+auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Lookup(
+    KeyType key) const -> IdT {
   if (auto result = set_.Lookup(key, KeyContext(&values_))) {
     return result.key();
   }
   return IdT::None;
 }
 
-template <typename IdT, typename KeyT, typename ValueT>
-auto CanonicalValueStore<IdT, KeyT, ValueT>::Reserve(size_t size) -> void {
+template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
+auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Reserve(size_t size)
+    -> void {
   // Compute the resulting new insert count using the size of values -- the
   // set doesn't have a fast to compute current size.
   if (size > values_.size()) {

toolchain/sem_ir/clang_decl.h

@@ -83,16 +83,6 @@ struct ClangDeclKey : public Printable<ClangDeclKey> {
 // Instances of this type are managed by a `ClangDeclStore`, which ensures that
 // a single `ClangDecl` exists for each `ClangDeclKey` used.
 struct ClangDecl : public Printable<ClangDecl> {
-  // Comparison against ClangDeclKey, required by CanonicalValueStore.
-  auto operator==(const ClangDeclKey& rhs) const -> bool { return key == rhs; }
-  auto operator==(const ClangDecl& rhs) const -> bool { return key == rhs.key; }
-
-  // Hashing for ClangDecl. See common/hashing.h.
-  friend auto CarbonHashValue(const ClangDecl& value, uint64_t seed)
-      -> HashCode {
-    return HashValue(value.key, seed);
-  }
-
   auto Print(llvm::raw_ostream& out) const -> void;
 
   // The key by which this declaration can be looked up.
@@ -118,7 +108,10 @@ struct ClangDeclId : public IdBase<ClangDeclId> {
 
 // Use the AST node pointer directly when doing `Lookup` to find an ID.
 using ClangDeclStore =
-    CanonicalValueStore<ClangDeclId, ClangDeclKey, ClangDecl>;
+    CanonicalValueStore<ClangDeclId, ClangDeclKey, ClangDecl,
+                        [](const ClangDecl& value) -> const ClangDeclKey& {
+                          return value.key;
+                        }>;
 
 }  // namespace Carbon::SemIR
 

toolchain/sem_ir/cpp_global_var.h

@@ -36,22 +36,6 @@ struct CppGlobalVar : public Printable<CppGlobalVar> {
     out << "{key: " << key << ", clang_decl_id: " << clang_decl_id << "}";
   }
 
-  // Comparison against `CppGlobalVarKey`, required by `CanonicalValueStore`.
-  friend auto operator==(const CppGlobalVar& lhs, const CppGlobalVarKey& rhs)
-      -> bool {
-    return lhs.key == rhs;
-  }
-  friend auto operator==(const CppGlobalVar& lhs, const CppGlobalVar& rhs)
-      -> bool {
-    return lhs.key == rhs.key;
-  }
-
-  // Hashing for `CppGlobalVar`. See common/hashing.h.
-  friend auto CarbonHashValue(const CppGlobalVar& value, uint64_t seed)
-      -> HashCode {
-    return HashValue(value.key, seed);
-  }
-
   // The key by which this variable can be looked up.
   CppGlobalVarKey key;
 
@@ -64,7 +48,9 @@ struct CppGlobalVar : public Printable<CppGlobalVar> {
 
 // Use the name of a C++ global variable when doing `Lookup` to find an ID.
 using CppGlobalVarStore =
-    CanonicalValueStore<CppGlobalVarId, CppGlobalVarKey, CppGlobalVar>;
+    CanonicalValueStore<CppGlobalVarId, CppGlobalVarKey, CppGlobalVar,
+                        [](const CppGlobalVar& value)
+                            -> const CppGlobalVarKey& { return value.key; }>;
 
 }  // namespace Carbon::SemIR