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

Code before:
  parent_scope.AddExtendedScope(constraint_inst_id);
}

// Pops the parameters of an `impl`, forming a `NameComponent` with no
// associated name that describes them.
static auto PopImplIntroducerAndParamsAsNameComponent(
    Context& context, Parse::AnyImplDeclId end_of_decl_node_id)
    -> NameComponent {
  auto [implicit_params_loc_id, implicit_param_patterns_id] =
      context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ImplForall>();

  if (implicit_param_patterns_id) {
    // Emit the `forall` match. This shouldn't produce any valid `Call` params,
    // because `impl`s are never actually called at runtime.
    auto call_params_id =
        CalleePatternMatch(context, *implicit_param_patterns_id,
                           SemIR::InstBlockId::Invalid, SemIR::InstId::Invalid);
    CARBON_CHECK(call_params_id == SemIR::InstBlockId::Empty ||
                 llvm::all_of(context.inst_blocks().Get(call_params_id),
                              [](SemIR::InstId inst_id) {
                                return inst_id ==
                                       SemIR::ErrorInst::SingletonInstId;
                              }));
  }

  Parse::Tree::PostorderIterator first_param_iter(
      context.node_stack().PopForSoloNodeId<Parse::NodeKind::ImplIntroducer>());
  // Subtracting 1 since we don't want to include the final `{` or `;` of the
  // declaration when performing syntactic match.
  auto node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
  CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart ||
               node_kind == Parse::NodeKind::ImplDecl);
  Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
  --last_param_iter;
  // Following proposal #3763, exclude a final `where` clause, if present. See:
  // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
  node_kind = context.parse_tree().node_kind(*last_param_iter);
  if (node_kind == Parse::NodeKind::WhereExpr) {
    int where_operands_to_skip = 1;
    --last_param_iter;
    CARBON_CHECK(first_param_iter < last_param_iter);
    do {
      node_kind = context.parse_tree().node_kind(*last_param_iter);
      if (node_kind == Parse::NodeKind::WhereExpr) {
        // If we have a nested `where`, we need to see another `WhereOperand`
        // before we find the one that matches our original `WhereExpr` node.
        ++where_operands_to_skip;
      } else if (node_kind == Parse::NodeKind::WhereOperand) {
        --where_operands_to_skip;
      }
      --last_param_iter;

Review comment (suggestion):
It looks like you wrap this just to write the CHECKs. Had you considered instead only doing the iterator construction inside the CHECK?

I'm particularly wondering about this because whereas `last_param_iter` is mutated, this isn't, so having it be an iterator and stored in `first_param_node_id` took me time to understand the code.

Code after:
  parent_scope.AddExtendedScope(constraint_inst_id);
}

// Pops the parameters of an `impl`, forming a `NameComponent` with no
// associated name that describes them.
static auto PopImplIntroducerAndParamsAsNameComponent(
    Context& context, Parse::AnyImplDeclId end_of_decl_node_id)
    -> NameComponent {
  auto [implicit_params_loc_id, implicit_param_patterns_id] =
      context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ImplForall>();

  if (implicit_param_patterns_id) {
    // Emit the `forall` match. This shouldn't produce any valid `Call` params,
    // because `impl`s are never actually called at runtime.
    auto call_params_id =
        CalleePatternMatch(context, *implicit_param_patterns_id,
                           SemIR::InstBlockId::Invalid, SemIR::InstId::Invalid);
    CARBON_CHECK(call_params_id == SemIR::InstBlockId::Empty ||
                 llvm::all_of(context.inst_blocks().Get(call_params_id),
                              [](SemIR::InstId inst_id) {
                                return inst_id ==
                                       SemIR::ErrorInst::SingletonInstId;
                              }));
  }

  Parse::NodeId first_param_node_id =
      context.node_stack().PopForSoloNodeId<Parse::NodeKind::ImplIntroducer>();

  // Subtracting 1 since we don't want to include the final `{` or `;` of the
  // declaration when performing syntactic match.
  auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
  CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart ||
               end_node_kind == Parse::NodeKind::ImplDecl);
  Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
  --last_param_iter;

  // Following proposal #3763, exclude a final `where` clause, if present. See:
  // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations

  // Caches the NodeKind for the current value of *last_param_iter so
  if (context.parse_tree().node_kind(*last_param_iter) ==
      Parse::NodeKind::WhereExpr) {
    int where_operands_to_skip = 1;
    --last_param_iter;
    CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
                 last_param_iter);
    do {
      auto node_kind = context.parse_tree().node_kind(*last_param_iter);
      if (node_kind == Parse::NodeKind::WhereExpr) {
        // If we have a nested `where`, we need to see another `WhereOperand`
        // before we find the one that matches our original `WhereExpr` node.