Address some review feedback.

- Introduced `CallbackID` typedef for callback ids rather than a plain
  int.
- Remove unnecessary braces.

Author: slackito

llvm/include/llvm/SandboxIR/Context.h

@@ -34,6 +34,9 @@ class Context {
   using MoveInstrCallback =
       std::function<void(Instruction *, const BBIterator &)>;
 
+  /// An ID for a registered callback. Used for deregistration.
+  using CallbackID = int;
+
 protected:
   LLVMContext &LLVMCtx;
   friend class Type;        // For LLVMCtx.
@@ -63,18 +66,18 @@ class Context {
 
   /// Callbacks called when an IR instruction is about to get removed. Keys are
   /// used as IDs for deregistration.
-  DenseMap<int, RemoveInstrCallback> RemoveInstrCallbacks;
+  DenseMap<CallbackID, RemoveInstrCallback> RemoveInstrCallbacks;
   /// Callbacks called when an IR instruction is about to get inserted. Keys are
   /// used as IDs for deregistration.
-  DenseMap<int, InsertInstrCallback> InsertInstrCallbacks;
+  DenseMap<CallbackID, InsertInstrCallback> InsertInstrCallbacks;
   /// Callbacks called when an IR instruction is about to get moved. Keys are
   /// used as IDs for deregistration.
-  DenseMap<int, MoveInstrCallback> MoveInstrCallbacks;
+  DenseMap<CallbackID, MoveInstrCallback> MoveInstrCallbacks;
 
   /// A counter used for assigning callback IDs during registration. The same
   /// counter is used for all kinds of callbacks so we can detect mismatched
   /// registration/deregistration.
-  static int NextCallbackId;
+  static CallbackID NextCallbackID;
 
   /// Remove \p V from the maps and returns the unique_ptr.
   std::unique_ptr<Value> detachLLVMValue(llvm::Value *V);

llvm/lib/SandboxIR/Context.cpp

@@ -664,56 +664,53 @@ Module *Context::createModule(llvm::Module *LLVMM) {
 }
 
 void Context::runRemoveInstrCallbacks(Instruction *I) {
-  for (const auto &CBEntry : RemoveInstrCallbacks) {
+  for (const auto &CBEntry : RemoveInstrCallbacks)
     CBEntry.second(I);
-  }
 }
 
 void Context::runInsertInstrCallbacks(Instruction *I) {
-  for (auto &CBEntry : InsertInstrCallbacks) {
+  for (auto &CBEntry : InsertInstrCallbacks)
     CBEntry.second(I);
-  }
 }
 
 void Context::runMoveInstrCallbacks(Instruction *I, const BBIterator &WhereIt) {
-  for (auto &CBEntry : MoveInstrCallbacks) {
+  for (auto &CBEntry : MoveInstrCallbacks)
     CBEntry.second(I, WhereIt);
-  }
 }
 
-int Context::NextCallbackId = 0;
+Context::CallbackID Context::NextCallbackID = 0;
 
 int Context::registerRemoveInstrCallback(RemoveInstrCallback CB) {
-  int Id = NextCallbackId++;
-  RemoveInstrCallbacks[Id] = CB;
-  return Id;
+  CallbackID ID = NextCallbackID++;
+  RemoveInstrCallbacks[ID] = CB;
+  return ID;
 }
-void Context::unregisterRemoveInstrCallback(int CallbackId) {
-  [[maybe_unused]] bool erased = RemoveInstrCallbacks.erase(CallbackId);
+void Context::unregisterRemoveInstrCallback(CallbackID ID) {
+  [[maybe_unused]] bool erased = RemoveInstrCallbacks.erase(ID);
   assert(erased &&
-         "Callback id not found in RemoveInstrCallbacks during deregistration");
+         "Callback ID not found in RemoveInstrCallbacks during deregistration");
 }
 
 int Context::registerInsertInstrCallback(InsertInstrCallback CB) {
-  int Id = NextCallbackId++;
-  InsertInstrCallbacks[Id] = CB;
-  return Id;
+  CallbackID ID = NextCallbackID++;
+  InsertInstrCallbacks[ID] = CB;
+  return ID;
 }
-void Context::unregisterInsertInstrCallback(int CallbackId) {
-  [[maybe_unused]] bool erased = InsertInstrCallbacks.erase(CallbackId);
+void Context::unregisterInsertInstrCallback(CallbackID ID) {
+  [[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID);
   assert(erased &&
-         "Callback id not found in InsertInstrCallbacks during deregistration");
+         "Callback ID not found in InsertInstrCallbacks during deregistration");
 }
 
 int Context::registerMoveInstrCallback(MoveInstrCallback CB) {
-  int Id = NextCallbackId++;
-  MoveInstrCallbacks[Id] = CB;
-  return Id;
+  CallbackID ID = NextCallbackID++;
+  MoveInstrCallbacks[ID] = CB;
+  return ID;
 }
-void Context::unregisterMoveInstrCallback(int CallbackId) {
-  [[maybe_unused]] bool erased = MoveInstrCallbacks.erase(CallbackId);
+void Context::unregisterMoveInstrCallback(CallbackID ID) {
+  [[maybe_unused]] bool erased = MoveInstrCallbacks.erase(ID);
   assert(erased &&
-         "Callback id not found in MoveInstrCallbacks during deregistration");
+         "Callback ID not found in MoveInstrCallbacks during deregistration");
 }
 
 } // namespace llvm::sandboxir

llvm/unittests/SandboxIR/SandboxIRTest.cpp

@@ -5980,18 +5980,18 @@ TEST_F(SandboxIRTest, InstructionCallbacks) {
   sandboxir::Instruction *Ret = &BB.front();
 
   SmallVector<sandboxir::Instruction *> Inserted;
-  int InsertCbId = Ctx.registerInsertInstrCallback(
+  auto InsertCbId = Ctx.registerInsertInstrCallback(
       [&Inserted](sandboxir::Instruction *I) { Inserted.push_back(I); });
 
   SmallVector<sandboxir::Instruction *> Removed;
-  int RemoveCbId = Ctx.registerRemoveInstrCallback(
+  auto RemoveCbId = Ctx.registerRemoveInstrCallback(
       [&Removed](sandboxir::Instruction *I) { Removed.push_back(I); });
 
   // Keep the moved instruction and the instruction pointed by the Where
   // iterator so we can check both callback arguments work as expected.
   SmallVector<std::pair<sandboxir::Instruction *, sandboxir::Instruction *>>
       Moved;
-  int MoveCbId = Ctx.registerMoveInstrCallback(
+  auto MoveCbId = Ctx.registerMoveInstrCallback(
       [&Moved](sandboxir::Instruction *I, const sandboxir::BBIterator &Where) {
         // Use a nullptr to signal "move to end" to keep it single. We only
         // have a basic block in this test case anyway.
@@ -6040,7 +6040,7 @@ TEST_F(SandboxIRTest, InstructionCallbacks) {
   Ctx.unregisterRemoveInstrCallback(RemoveCbId);
   Ctx.unregisterMoveInstrCallback(MoveCbId);
   auto *NewI2 = sandboxir::StoreInst::create(Val, Ptr, /*Align=*/std::nullopt,
-                                            Ret->getIterator(), Ctx);
+                                             Ret->getIterator(), Ctx);
   Ret->moveBefore(NewI2);
   Ret->eraseFromParent();
   EXPECT_THAT(Inserted, testing::IsEmpty());