Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/formatter.cpp (C++)

Code before:
}

auto Formatter::FormatTopLevelScopeIfUsed(InstNamer::ScopeId scope_id,
                                          llvm::ArrayRef<InstId> block,
                                          bool is_tentative) -> void {
  if (use_dump_sem_ir_ranges_) {
    // Don't format the scope if no instructions are in a dump range.
    block = block.drop_while(
        [&](InstId inst_id) { return !ShouldFormatInst(inst_id); });
  }

  if (block.empty()) {
    return;
  }

  llvm::SaveAndRestore scope(scope_, scope_id);
  // Note, we don't use OpenBrace() / CloseBrace() here because we always want
  // a newline to avoid misformatting if the first instruction is omitted.
  out_ << "\n" << inst_namer_.GetScopeName(scope_id) << " {\n";
  indent_ += 2;
  for (const InstId inst_id : block) {
    // Unlike code blocks, we don't note when a scope elide entries, because
    // they're less likely to be related to the elided code.
    if (ShouldFormatInst(inst_id)) {
      if (is_tentative) {
        TentativeOutputScope scope(*this,
                                   tentative_inst_chunks_[inst_id.index]);
        FormatInst(inst_id);
      } else {
        FormatInst(inst_id);
      }
    }
  }
  out_ << "}\n";
  indent_ -= 2;
}

auto Formatter::FormatClass(ClassId id) -> void {
  const Class& class_info = sem_ir_->classes().Get(id);
  if (!ShouldFormatEntity(class_info)) {
    return;
  }

  FormatEntityStart("class", class_info, id);

  llvm::SaveAndRestore class_scope(scope_, inst_namer_.GetScopeFor(id));

  if (class_info.scope_id.has_value()) {
    out_ << ' ';
    OpenBrace();
    FormatCodeBlock(class_info.body_block_id);

Review comment (suggestion):
According to their comments, `ShouldFormatInst` queries whether the inst should be included in the output, and `TentativeOutputScope` is for when we don't know whether it's included in the output. So once `ShouldFormatInst` has told us it be output, why do we still need to be tentative about it? It seems like there are two different notions of whether an inst should be included in the output, and I don't understand the difference between them.

Code after:
}

auto Formatter::FormatTopLevelScopeIfUsed(InstNamer::ScopeId scope_id,
                                          llvm::ArrayRef<InstId> block,
                                          bool use_tentative_output_scopes)
    -> void {
  if (!use_tentative_output_scopes && use_dump_sem_ir_ranges_) {
    // Don't format the scope if no instructions are in a dump range.
    block = block.drop_while(
        [&](InstId inst_id) { return !ShouldFormatInst(inst_id); });
  }

  if (block.empty()) {
    return;
  }

  llvm::SaveAndRestore scope(scope_, scope_id);
  // Note, we don't use OpenBrace() / CloseBrace() here because we always want
  // a newline to avoid misformatting if the first instruction is omitted.
  out_ << "\n" << inst_namer_.GetScopeName(scope_id) << " {\n";
  indent_ += 2;
  for (const InstId inst_id : block) {
    // Format instructions when needed, but do nothing for elided entries;
    // unlike normal code blocks, scopes are non-sequential so skipped
    // instructions are assumed to be uninteresting.
    if (use_tentative_output_scopes) {
      // This is for constants and imports. These use tentative logic to
      // determine whether an instruction is printed.
      TentativeOutputScope scope(*this, tentative_inst_chunks_[inst_id.index]);
      FormatInst(inst_id);
    } else if (ShouldFormatInst(inst_id)) {
      // This is for the file scope. It uses only the range-based filtering.
      FormatInst(inst_id);
    }
  }
  out_ << "}\n";
  indent_ -= 2;
}

auto Formatter::FormatClass(ClassId id) -> void {
  const Class& class_info = sem_ir_->classes().Get(id);
  if (!ShouldFormatEntity(class_info)) {
    return;
  }

  FormatEntityStart("class", class_info, id);

  llvm::SaveAndRestore class_scope(scope_, inst_namer_.GetScopeFor(id));

  if (class_info.scope_id.has_value()) {
    out_ << ' ';