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

Code before:
  if (base_type_id.has_value()) {
    auto base_decl = context.insts().GetAs<SemIR::BaseDecl>(class_info.base_id);
    base_decl.index =
        SemIR::ElementIndex{static_cast<int>(struct_type_fields.size())};
    ReplaceInstPreservingConstantValue(context, class_info.base_id, base_decl);
    struct_type_fields.push_back(
        {.name_id = SemIR::NameId::Base, .type_id = base_type_id});
  }

  if (class_info.is_dynamic) {
    llvm::SmallVector<SemIR::InstId> vtable;
    bool vtable_incomplete_due_to_error = false;
    if (!defining_vptr) {
      LoadImportRef(context, base_class_info->vtable_id);
      auto base_vtable_id = context.constant_values().GetConstantInstId(
          base_class_info->vtable_id);
      if (base_vtable_id == SemIR::ErrorInst::SingletonInstId) {
        vtable_incomplete_due_to_error = true;
      }
      auto base_vtable_inst_block =
          vtable_incomplete_due_to_error
              ? llvm::ArrayRef<SemIR::InstId>{}
              : 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 (refactor):
This ternary seems pretty big. But, it looks a lot like this variable is only used by the for loop. Had you considered just moving `base_vtable_inst_block` and the `for` loop into an `else` clause of the added `if`, or `if (base_vtable_id != SemIR::ErrorInst::SingletonInstId) {`?

i.e.:

```
      if (base_vtable_id == SemIR::ErrorInst::SingletonInstId) {
        vtable_incomplete_due_to_error = true;
      } else {
        auto base_vtable_inst_block = ...
        for (auto fn_decl_id : base_vtable_inst_block) {
          ...
        }
      }
```

Or, it seems like `vtable` is built then discarded; maybe the whole construction of `vtable` should short-circuit including `for (auto inst_id : context.vtable_stack().PeekCurrentBlockContents()) {`? Perhaps a helper function would make the flow that's happening here clearer, e.g. with an early return on error?


Code after:
                                   SemIR::SpecificId::None,
                                   /*check_syntax=*/false,
                                   /*check_self=*/false);
          fn_decl_id = override_fn_decl_id;
        }
      }
      vtable.push_back(fn_decl_id);
    }
  }

  for (auto inst_id : context.vtable_stack().PeekCurrentBlockContents()) {
    auto fn_decl = context.insts().GetAs<SemIR::FunctionDecl>(inst_id);
    const auto& fn = context.functions().Get(fn_decl.function_id);
    if (fn.virtual_modifier != SemIR::FunctionFields::VirtualModifier::Impl) {
      vtable.push_back(inst_id);
    }
  }
  return AddInst<SemIR::Vtable>(
      context, node_id,
      {.type_id = GetSingletonType(context, SemIR::VtableType::SingletonInstId),
       .virtual_functions_id = context.inst_blocks().Add(vtable)});
}

// Checks that the specified finished class definition is valid and builds and
// returns a corresponding complete type witness instruction.
static auto CheckCompleteClassType(Context& context, Parse::NodeId node_id,
                                   SemIR::ClassId class_id) -> SemIR::InstId {
  auto& class_info = context.classes().Get(class_id);
  if (class_info.adapt_id.has_value()) {
    return CheckCompleteAdapterClassType(context, node_id, class_id);
  }

  bool defining_vptr = class_info.is_dynamic;
  auto base_type_id =
      class_info.GetBaseType(context.sem_ir(), SemIR::SpecificId::None);
  SemIR::Class* base_class_info = nullptr;
  if (base_type_id.has_value()) {
    // TODO: If the base class is template dependent, we will need to decide
    // whether to add a vptr as part of instantiation.
    base_class_info = TryGetAsClass(context, base_type_id);
    if (base_class_info && base_class_info->is_dynamic) {
      defining_vptr = false;
    }
  }

  auto field_decls = context.field_decls_stack().PeekArray();
  llvm::SmallVector<SemIR::StructTypeField> struct_type_fields;
  struct_type_fields.reserve(defining_vptr + class_info.base_id.has_value() +
                             field_decls.size());
  if (defining_vptr) {
    struct_type_fields.push_back(