From 3ec1383efe58685cf26bec94e807b2b45f66d190 Mon Sep 17 00:00:00 2001
From: Boaz Brickner <brickner@google.com>
Date: Wed, 25 Jun 2025 16:22:43 +0200
Subject: [PATCH] Customize `ClangDeclId` instead of customizing `ClangDecl`
 hashing and equality This keeps the principal of hashing and equality of a
 type includes all its values.

Done by extending `CanonicalValueStore` to support calling a `TranslateValueToKey()` function defined in the `Id`, which converts the value to the key used for hashing.

This is a draft.

TODO:
* Create a `ClangDeclStore` (possibly in a separate PR), to make its API clearer to use.
* Add and update documentation.
* Add tests.
---
 toolchain/base/value_store.h  | 17 +++++++++++++++--
 toolchain/sem_ir/clang_decl.h | 14 +++-----------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h
index d1c70187228c0..7d2bc66123ec2 100644
--- a/toolchain/base/value_store.h
+++ b/toolchain/base/value_store.h
@@ -267,6 +267,9 @@ class CanonicalValueStore {
   Set<IdT, /*SmallSize=*/0, KeyContext> set_;
 };
 
+template <typename T>
+concept HasTranslateValueToKey = requires { &T::TranslateValueToKey; };
+
 template <typename IdT>
 class CanonicalValueStore<IdT>::KeyContext
     : public TranslatingKeyContext<KeyContext> {
@@ -275,8 +278,18 @@ class CanonicalValueStore<IdT>::KeyContext
 
   // 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 -> ValueStore<IdT>::ConstRefType {
-    return values_->Get(id);
+  auto TranslateKey(IdT id) const {
+    if constexpr (requires { IdT::TranslateValueToKey(values_->Get(id)); }) {
+      return IdT::TranslateValueToKey(values_->Get(id));
+    } else {
+      return values_->Get(id);
+    }
+  }
+
+  auto TranslateKey(ValueType value) const
+    requires HasTranslateValueToKey<IdT>
+  {
+    return IdT::TranslateValueToKey(value);
   }
 
  private:
diff --git a/toolchain/sem_ir/clang_decl.h b/toolchain/sem_ir/clang_decl.h
index d918f92b49786..708235a7aec30 100644
--- a/toolchain/sem_ir/clang_decl.h
+++ b/toolchain/sem_ir/clang_decl.h
@@ -28,17 +28,6 @@ namespace Carbon::SemIR {
 struct ClangDecl : public Printable<ClangDecl> {
   auto Print(llvm::raw_ostream& out) const -> void;
 
-  friend auto CarbonHashtableEq(const ClangDecl& lhs, const ClangDecl& rhs)
-      -> bool {
-    return HashtableEq(lhs.decl, rhs.decl);
-  }
-
-  // Hashing for ClangDecl. See common/hashing.h.
-  friend auto CarbonHashValue(const ClangDecl& value, uint64_t seed)
-      -> HashCode {
-    return HashValue(value.decl, seed);
-  }
-
   // The Clang declaration pointing to the Clang AST.
   // TODO: Ensure we can easily serialize/deserialize this. Consider
   // `clang::LazyDeclPtr`.
@@ -53,6 +42,9 @@ struct ClangDeclId : public IdBase<ClangDeclId> {
   static constexpr llvm::StringLiteral Label = "clang_decl_id";
 
   using ValueType = ClangDecl;
+  static auto TranslateValueToKey(const ValueType& value) -> clang::Decl* {
+    return value.decl;
+  }
 
   using IdBase::IdBase;
 };