Repository: carbon-language/carbon-lang
File: toolchain/check/handle_class.cpp (C++)

Code before:
        context.sem_ir().types().GetAs<SemIR::UnboundElementType>(
            field_decl.type_id);
    struct_type_fields.push_back(
        {.name_id = field_decl.name_id,
         .type_id = unbound_element_type.element_type_id});
  }
  auto fields_id =
      context.struct_type_fields().AddCanonical(struct_type_fields);
  return fields_id;
}

// Builds and returns a vtable for the current class. Assumes that the virtual
// functions for the class are listed as the top element of the `vtable_stack`.
static auto BuildVtable(Context& context, Parse::NodeId node_id,
                        SemIR::InstId base_vtable_id) -> SemIR::InstId {
  llvm::SmallVector<SemIR::InstId> vtable;
  if (base_vtable_id.has_value()) {
    LoadImportRef(context, base_vtable_id);
    base_vtable_id =
        context.constant_values().GetConstantInstId(base_vtable_id);
    if (base_vtable_id == SemIR::ErrorInst::SingletonInstId) {
      return SemIR::ErrorInst::SingletonInstId;
    }
    auto base_vtable_inst_block =
        context.inst_blocks().Get(context.insts()
                                      .GetAs<SemIR::Vtable>(base_vtable_id)
                                      .virtual_functions_id);
    // TODO: Avoid quadratic search. Perhaps build a map from `NameId` to the
    // elements of the top of `vtable_stack`.
    for (auto fn_decl_id : base_vtable_inst_block) {
      auto fn_decl = GetCalleeFunction(context.sem_ir(), fn_decl_id);
      const auto& fn = context.functions().Get(fn_decl.function_id);
      for (auto override_fn_decl_id :
           context.vtable_stack().PeekCurrentBlockContents()) {
        auto override_fn_decl =
            context.insts().GetAs<SemIR::FunctionDecl>(override_fn_decl_id);
        const auto& override_fn =
            context.functions().Get(override_fn_decl.function_id);
        if (override_fn.virtual_modifier ==
                SemIR::FunctionFields::VirtualModifier::Impl &&
            override_fn.name_id == fn.name_id) {
          // TODO: Support generic base classes, rather than passing
          // `SpecificId::None`.
          CheckFunctionTypeMatches(context, override_fn, fn,
                                   SemIR::SpecificId::None,
                                   /*check_syntax=*/false,
                                   /*check_self=*/false);
          fn_decl_id = override_fn_decl_id;
        }
      }
      vtable.push_back(fn_decl_id);

Review comment (suggestion):
Maybe declare a new name? Overwriting the value makes me assume that the original value would be used in some way (when not overwritten), which doesn't seem to be the case here.

```suggestion
    auto canonical_vtable_id =
        context.constant_values().GetConstantInstId(base_vtable_id);
    if (canonical_vtable_id == SemIR::ErrorInst::SingletonInstId) {
      return SemIR::ErrorInst::SingletonInstId;
    }
    auto base_vtable_inst_block =
        context.inst_blocks().Get(context.insts()
                                      .GetAs<SemIR::Vtable>(canonical_vtable_id)
```


Code after:
        context.sem_ir().types().GetAs<SemIR::UnboundElementType>(
            field_decl.type_id);
    struct_type_fields.push_back(
        {.name_id = field_decl.name_id,
         .type_id = unbound_element_type.element_type_id});
  }
  auto fields_id =
      context.struct_type_fields().AddCanonical(struct_type_fields);
  return fields_id;
}

// Builds and returns a vtable for the current class. Assumes that the virtual
// functions for the class are listed as the top element of the `vtable_stack`.
static auto BuildVtable(Context& context, Parse::NodeId node_id,
                        SemIR::InstId base_vtable_id) -> SemIR::InstId {
  llvm::SmallVector<SemIR::InstId> vtable;
  if (base_vtable_id.has_value()) {
    LoadImportRef(context, base_vtable_id);
    auto canonical_base_vtable_id =
        context.constant_values().GetConstantInstId(base_vtable_id);
    if (canonical_base_vtable_id == SemIR::ErrorInst::SingletonInstId) {
      return SemIR::ErrorInst::SingletonInstId;
    }
    auto base_vtable_inst_block = context.inst_blocks().Get(
        context.insts()
            .GetAs<SemIR::Vtable>(canonical_base_vtable_id)
            .virtual_functions_id);
    // TODO: Avoid quadratic search. Perhaps build a map from `NameId` to the
    // elements of the top of `vtable_stack`.
    for (auto fn_decl_id : base_vtable_inst_block) {
      auto fn_decl = GetCalleeFunction(context.sem_ir(), fn_decl_id);
      const auto& fn = context.functions().Get(fn_decl.function_id);
      for (auto override_fn_decl_id :
           context.vtable_stack().PeekCurrentBlockContents()) {
        auto override_fn_decl =
            context.insts().GetAs<SemIR::FunctionDecl>(override_fn_decl_id);
        const auto& override_fn =
            context.functions().Get(override_fn_decl.function_id);
        if (override_fn.virtual_modifier ==
                SemIR::FunctionFields::VirtualModifier::Impl &&
            override_fn.name_id == fn.name_id) {
          // TODO: Support generic base classes, rather than passing
          // `SpecificId::None`.
          CheckFunctionTypeMatches(context, override_fn, fn,
                                   SemIR::SpecificId::None,
                                   /*check_syntax=*/false,
                                   /*check_self=*/false);
          fn_decl_id = override_fn_decl_id;
        }
      }
      vtable.push_back(fn_decl_id);