ronantakizawa/codereview-bench huggingface dataset:
Repository: carbon-language/carbon-lang
File: testing/file_test/test_file.cpp (C++)

Code before:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "testing/file_test/test_file.h"

#include <fstream>

#include "common/check.h"
#include "common/error.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/JSON.h"
#include "testing/base/file_helpers.h"

namespace Carbon::Testing {

using ::testing::Matcher;
using ::testing::MatchesRegex;
using ::testing::StrEq;

// Processes conflict markers, including tracking of whether code is within a
// conflict marker. Returns true if the line is consumed.
static auto TryConsumeConflictMarker(bool running_autoupdate,
                                     llvm::StringRef line,
                                     llvm::StringRef line_trimmed,
                                     bool* inside_conflict_marker)
    -> ErrorOr<bool> {
  bool is_start = line.starts_with("<<<<<<<");
  bool is_middle = line.starts_with("=======") || line.starts_with("|||||||");
  bool is_end = line.starts_with(">>>>>>>");

  // When running the test, any conflict marker is an error.
  if (!running_autoupdate && (is_start || is_middle || is_end)) {
    return ErrorBuilder() << "Conflict marker found:\n" << line;
  }

Review comment (suggestion):
```suggestion
#include "common/error.h"
#include "common/raw_string_ostream.h"
```

(See RawStringOstream comment below)

Code after:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "testing/file_test/test_file.h"

#include <fstream>

#include "common/check.h"
#include "common/error.h"
#include "common/raw_string_ostream.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/JSON.h"
#include "testing/base/file_helpers.h"

namespace Carbon::Testing {

using ::testing::Matcher;
using ::testing::MatchesRegex;
using ::testing::StrEq;

// Processes conflict markers, including tracking of whether code is within a
// conflict marker. Returns true if the line is consumed.
static auto TryConsumeConflictMarker(bool running_autoupdate,
                                     llvm::StringRef line,
                                     llvm::StringRef line_trimmed,
                                     bool* inside_conflict_marker)
    -> ErrorOr<bool> {
  bool is_start = line.starts_with("<<<<<<<");
  bool is_middle = line.starts_with("=======") || line.starts_with("|||||||");
  bool is_end = line.starts_with(">>>>>>>");

  // When running the test, any conflict marker is an error.
  if (!running_autoupdate && (is_start || is_middle || is_end)) {
    return ErrorBuilder() << "Conflict marker found:\n" << line;
Repository: carbon-language/carbon-lang
File: toolchain/check/type_structure.cpp (C++)

Code before:
    CloseWithConcrete,
    CloseWithSymbolic,
  };
  struct CloseType {
    CloseTypeClosing closing = CloseOnly;
  };
  struct SymbolicType {};
  struct NonTypeValue {};

  using WorkItem = std::variant<SemIR::TypeId, SymbolicType, NonTypeValue,
                                SemIR::SpecificInterface, CloseType>;

  auto TryGetInstIdAsTypeId(SemIR::InstId inst_id) const
      -> std::variant<SemIR::TypeId, SymbolicType> {
    if (auto facet_value =
            context_.insts().TryGetAs<SemIR::FacetValue>(inst_id)) {
      inst_id = facet_value->type_inst_id;
    }

    auto type_id_of_inst_id = context_.insts().Get(inst_id).type_id();
    if (context_.types().Is<SemIR::FacetType>(type_id_of_inst_id)) {
      return SymbolicType();
    }
    if (type_id_of_inst_id != SemIR::TypeType::SingletonTypeId) {
      return SemIR::TypeId::None;
    }
    return context_.types().GetTypeIdForTypeInstId(inst_id);
  }

  // Sets the `distance` in `first_symbolic_distance_` if it does not already
  // have a non-infinite value.
  auto SetFirstSymbolic(int distance) -> void {
    if (first_symbolic_distance_ == TypeStructure::InfiniteDistance) {
      first_symbolic_distance_ = distance;
    }
  }

  auto GetSpecificArgs(SemIR::SpecificId specific_id)
      -> llvm::ArrayRef<SemIR::InstId> {
    if (specific_id == SemIR::SpecificId::None) {
      return {};
    }
    auto specific = context_.specifics().Get(specific_id);
    return context_.inst_blocks().Get(specific.args_id);
  }

  // Push all arguments from the array.
  auto PushArgs(llvm::ArrayRef<SemIR::InstId> args) -> void {
    for (auto arg_id : llvm::reverse(args)) {
      auto maybe_type_id = TryGetInstIdAsTypeId(arg_id);
      if (std::holds_alternative<SymbolicType>(maybe_type_id)) {

Review comment (suggestion):
A comment explaining why it's correct to treat non-type facets as always being symbolic would be useful. (I think this might be right, but I'm not certain.)

Code after:
      Push(type_id);
    } else {
      Push(NonTypeValue());
    }
  }

  // Push the next step into the work queue.
  auto Push(WorkItem item) -> void { work_list_.push_back(item); }

  // Append a structural element to the TypeStructure being built.
  auto AppendStructural(TypeStructure::Structural structural) -> void {
    if (structural == TypeStructure::Structural::Symbolic) {
      // Sets the `distance` in `first_symbolic_distance_` if it does not
      // already have a non-infinite value.
      if (first_symbolic_distance_ == TypeStructure::InfiniteDistance) {
        first_symbolic_distance_ = structure_.size();
      }
    }
    structure_.push_back(structural);
  }

  Context& context_;
  llvm::SmallVector<WorkItem> work_list_;
  int first_symbolic_distance_;
  llvm::SmallVector<TypeStructure::Structural> structure_;
};

auto BuildTypeStructure(Context& context, SemIR::InstId self_inst_id,
                        SemIR::SpecificInterface interface) -> TypeStructure {
  TypeStructureBuilder builder(context);
  return builder.Run(self_inst_id, interface);
}

}  // namespace Carbon::Check
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/dump.cpp (C++)

Code before:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef NDEBUG

#include "toolchain/sem_ir/dump.h"

#include "common/raw_string_ostream.h"
#include "toolchain/sem_ir/stringify_type.h"

namespace Carbon::SemIR {

static auto DumpNameIfValid(const File& file, NameId name_id) -> std::string {
  RawStringOstream out;
  if (name_id.has_value()) {
    out << " `" << file.names().GetFormatted(name_id) << "`";
  }
  return out.TakeStr();
}

static auto DumpConstSummary(const File& file, ConstantId const_id)
    -> std::string {
  RawStringOstream out;
  out << const_id;
  if (!const_id.has_value()) {
    return out.TakeStr();
  }
  if (const_id.is_symbolic()) {
    out << ": " << file.constant_values().GetSymbolicConstant(const_id);
  } else if (const_id.is_concrete()) {
    out << ": " << file.insts().Get(file.constant_values().GetInstId(const_id));
  }
  return out.TakeStr();
}

static auto DumpGenericSummary(const File& file, GenericId generic_id)
    -> std::string {
  RawStringOstream out;
  out << generic_id;
  if (!generic_id.has_value()) {
    return out.TakeStr();
  }
  const auto& generic = file.generics().Get(generic_id);
  out << ": " << generic << '\n' << Dump(file, generic.decl_id);
  return out.TakeStr();
}

Review comment (suggestion):
nit:
```suggestion
static auto DumpConstantSummary(const File& file, ConstantId const_id)
```

Code after:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef NDEBUG

#include "toolchain/sem_ir/dump.h"

#include "common/raw_string_ostream.h"
#include "toolchain/sem_ir/stringify_type.h"

namespace Carbon::SemIR {

static auto DumpNameIfValid(const File& file, NameId name_id) -> std::string {
  RawStringOstream out;
  if (name_id.has_value()) {
    out << " `" << file.names().GetFormatted(name_id) << "`";
  }
  return out.TakeStr();
}

static auto DumpConstantSummary(const File& file, ConstantId const_id)
    -> std::string {
  RawStringOstream out;
  out << const_id;
  if (!const_id.has_value()) {
    return out.TakeStr();
  }
  if (const_id.is_symbolic()) {
    out << ": " << file.constant_values().GetSymbolicConstant(const_id);
  } else if (const_id.is_concrete()) {
    out << ": " << file.insts().Get(file.constant_values().GetInstId(const_id));
  }
  return out.TakeStr();
}

static auto DumpGenericSummary(const File& file, GenericId generic_id)
    -> std::string {
  RawStringOstream out;
  out << generic_id;
  if (!generic_id.has_value()) {
    return out.TakeStr();
  }
  const auto& generic = file.generics().Get(generic_id);
  out << ": " << generic << "\ndecl: " << Dump(file, generic.decl_id);
  return out.TakeStr();
}
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/dump.cpp (C++)

Code before:
}

static auto DumpConstSummary(const File& file, ConstantId const_id)
    -> std::string {
  RawStringOstream out;
  out << const_id;
  if (!const_id.has_value()) {
    return out.TakeStr();
  }
  if (const_id.is_symbolic()) {
    out << ": " << file.constant_values().GetSymbolicConstant(const_id);
  } else if (const_id.is_concrete()) {
    out << ": " << file.insts().Get(file.constant_values().GetInstId(const_id));
  }
  return out.TakeStr();
}

static auto DumpGenericSummary(const File& file, GenericId generic_id)
    -> std::string {
  RawStringOstream out;
  out << generic_id;
  if (!generic_id.has_value()) {
    return out.TakeStr();
  }
  const auto& generic = file.generics().Get(generic_id);
  out << ": " << generic << '\n' << Dump(file, generic.decl_id);
  return out.TakeStr();
}

static auto DumpInstSummary(const File& file, InstId inst_id) -> std::string {
  RawStringOstream out;
  out << inst_id;
  if (inst_id.has_value()) {
    out << ": " << file.insts().Get(inst_id);
  }
  return out.TakeStr();
}

static auto DumpSpecificSummary(const File& file, SpecificId specific_id)
    -> std::string {
  RawStringOstream out;
  out << specific_id;
  if (specific_id.has_value()) {
    out << ": " << file.specifics().Get(specific_id);
  }
  return out.TakeStr();
}

LLVM_DUMP_METHOD auto Dump(const File& file, ClassId class_id) -> std::string {
  RawStringOstream out;
  out << class_id;

Review comment (suggestion):
```suggestion
  out << ": " << generic << "\ndecl: " << Dump(file, generic.decl_id);
```

Code after:
}

static auto DumpConstantSummary(const File& file, ConstantId const_id)
    -> std::string {
  RawStringOstream out;
  out << const_id;
  if (!const_id.has_value()) {
    return out.TakeStr();
  }
  if (const_id.is_symbolic()) {
    out << ": " << file.constant_values().GetSymbolicConstant(const_id);
  } else if (const_id.is_concrete()) {
    out << ": " << file.insts().Get(file.constant_values().GetInstId(const_id));
  }
  return out.TakeStr();
}

static auto DumpGenericSummary(const File& file, GenericId generic_id)
    -> std::string {
  RawStringOstream out;
  out << generic_id;
  if (!generic_id.has_value()) {
    return out.TakeStr();
  }
  const auto& generic = file.generics().Get(generic_id);
  out << ": " << generic << "\ndecl: " << Dump(file, generic.decl_id);
  return out.TakeStr();
}

static auto DumpInstSummary(const File& file, InstId inst_id) -> std::string {
  RawStringOstream out;
  out << inst_id;
  if (inst_id.has_value()) {
    out << ": " << file.insts().Get(inst_id);
  }
  return out.TakeStr();
}

static auto DumpSpecificSummary(const File& file, SpecificId specific_id)
    -> std::string {
  RawStringOstream out;
  out << specific_id;
  if (specific_id.has_value()) {
    out << ": " << file.specifics().Get(specific_id);
  }
  return out.TakeStr();
}

LLVM_DUMP_METHOD auto Dump(const File& file, ClassId class_id) -> std::string {
  RawStringOstream out;
  out << class_id;
Repository: carbon-language/carbon-lang
File: toolchain/lower/file_context.h (C/C++)

Code before:
  llvm::SmallVector<llvm::Function*, 0> specific_functions_;

  // Maps which specific functions are generics that need to have their
  // definitions lowered after the lowering of other definitions.
  // This list may grow while lowering generic definitions from this list.
  // The list uses the `SpecificId` to index into specific_functions_.
  llvm::SmallVector<std::pair<SemIR::FunctionId, SemIR::SpecificId>, 10>
      specific_function_definitions_;

  // Provides lowered versions of types.
  // Vector indexes correspond to `TypeId` indexes for non-symbolic types. We
  // resize this directly to the (often large) correct size.
  llvm::SmallVector<llvm::Type*, 0> types_;

  // Lowered version of the builtin type `type`.
  llvm::StructType* type_type_ = nullptr;

  // Maps constants to their lowered values.
  // Vector indexes correspond to `InstId` indexes for constant instructions. We
  // resize this directly to the (often large) correct size.
  llvm::SmallVector<llvm::Constant*, 0> constants_;

  // Maps global variables to their lowered variant.
  Map<SemIR::InstId, llvm::GlobalVariable*> global_variables_;

  // Global format string for PrintInt builtin.
  llvm::Value* printf_int_format_string_ = nullptr;
};

}  // namespace Carbon::Lower

#endif  // CARBON_TOOLCHAIN_LOWER_FILE_CONTEXT_H_


Review comment (suggestion):
```suggestion
  // Global format string for "printf.int.format" used by the PrintInt builtin.
```

Code after:
  llvm::SmallVector<llvm::Function*, 0> specific_functions_;

  // Maps which specific functions are generics that need to have their
  // definitions lowered after the lowering of other definitions.
  // This list may grow while lowering generic definitions from this list.
  // The list uses the `SpecificId` to index into specific_functions_.
  llvm::SmallVector<std::pair<SemIR::FunctionId, SemIR::SpecificId>, 10>
      specific_function_definitions_;

  // Provides lowered versions of types.
  // Vector indexes correspond to `TypeId` indexes for non-symbolic types. We
  // resize this directly to the (often large) correct size.
  llvm::SmallVector<llvm::Type*, 0> types_;

  // Lowered version of the builtin type `type`.
  llvm::StructType* type_type_ = nullptr;

  // Maps constants to their lowered values.
  // Vector indexes correspond to `InstId` indexes for constant instructions. We
  // resize this directly to the (often large) correct size.
  llvm::SmallVector<llvm::Constant*, 0> constants_;

  // Maps global variables to their lowered variant.
  Map<SemIR::InstId, llvm::GlobalVariable*> global_variables_;

  // Global format string for `printf.int.format` used by the PrintInt builtin.
  llvm::Value* printf_int_format_string_ = nullptr;
};

}  // namespace Carbon::Lower

#endif  // CARBON_TOOLCHAIN_LOWER_FILE_CONTEXT_H_
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/inst_kind.cpp (C++)

Code before:
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "toolchain/sem_ir/inst_kind.h"

#include "toolchain/sem_ir/typed_insts.h"

namespace Carbon::SemIR {

CARBON_DEFINE_ENUM_CLASS_NAMES(InstKind) = {
#define CARBON_SEM_IR_INST_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name)
#include "toolchain/sem_ir/inst_kind.def"
};

auto InstKind::definition_info(InstKind inst_kind) -> const DefinitionInfo& {
  static constexpr InstKind::DefinitionInfo DefinitionInfos[] = {
#define CARBON_SEM_IR_INST_KIND(Name) Name::Kind.info_,
#include "toolchain/sem_ir/inst_kind.def"
  };
  return DefinitionInfos[inst_kind.AsInt()];
}

auto InstKind::value_kind() const -> InstValueKind {
  static constexpr InstValueKind Table[] = {
#define CARBON_SEM_IR_INST_KIND(Name) \
  Internal::HasTypeIdMember<Name> ? InstValueKind::Typed : InstValueKind::None,
#include "toolchain/sem_ir/inst_kind.def"
  };
  return Table[AsInt()];
}

}  // namespace Carbon::SemIR


Review comment (suggestion):
The namespace qualification in this case is necessary: the enum-like type `InstKind` has members whose names are the same as the typed instructions, and the `SemIR::` disambiguates. (This PR doesn't build because of this.)

Code after:
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "toolchain/sem_ir/inst_kind.h"

#include "toolchain/sem_ir/typed_insts.h"

namespace Carbon::SemIR {

CARBON_DEFINE_ENUM_CLASS_NAMES(InstKind) = {
#define CARBON_SEM_IR_INST_KIND(Name) CARBON_ENUM_CLASS_NAME_STRING(Name)
#include "toolchain/sem_ir/inst_kind.def"
};

auto InstKind::definition_info(InstKind inst_kind) -> const DefinitionInfo& {
  static constexpr InstKind::DefinitionInfo DefinitionInfos[] = {
#define CARBON_SEM_IR_INST_KIND(Name) SemIR::Name::Kind.info_,
#include "toolchain/sem_ir/inst_kind.def"
  };
  return DefinitionInfos[inst_kind.AsInt()];
}

auto InstKind::value_kind() const -> InstValueKind {
  static constexpr InstValueKind Table[] = {
#define CARBON_SEM_IR_INST_KIND(Name)                           \
  Internal::HasTypeIdMember<SemIR::Name> ? InstValueKind::Typed \
                                         : InstValueKind::None,
#include "toolchain/sem_ir/inst_kind.def"
  };
  return Table[AsInt()];
}

}  // namespace Carbon::SemIR
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/import_ir.h (C/C++)

Code before:
                : lhs.inst_id() == rhs.inst_id());
  }

  auto ir_id() const -> ImportIRId { return ir_id_; }
  auto inst_id() const -> InstId {
    CARBON_CHECK(ir_id() != ImportIRId::Cpp);
    return inst_id_;
  }
  auto clang_source_loc_id() const -> ClangSourceLocId {
    CARBON_CHECK(ir_id() == ImportIRId::Cpp);
    return clang_source_loc_id_;
  }

 private:
  ImportIRId ir_id_;
  union {
    // Set iff `ir_id != ImportIRId::Cpp`.
    InstId inst_id_;

    // Set iff `ir_id == ImportIRId::Cpp`.
    ClangSourceLocId clang_source_loc_id_;
  };
};

// Returns the canonical `File` and `InstId` for an entity, tracing imported
// instructions. Note the returned `File` may not be directly imported by the
// input `sem_ir`.
auto GetCanonicalFileAndInstId(const File* sem_ir, SemIR::InstId inst_id)
    -> std::pair<const File*, InstId>;

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_IMPORT_IR_H_


Review comment (suggestion):
```suggestion
// instructions. Note the returned `File` might not be directly imported by the
```
(to avoid the ambiguity that "may not" can either mean "is not allowed to" or "might sometimes not")

Code after:
                : lhs.inst_id() == rhs.inst_id());
  }

  auto ir_id() const -> ImportIRId { return ir_id_; }
  auto inst_id() const -> InstId {
    CARBON_CHECK(ir_id() != ImportIRId::Cpp);
    return inst_id_;
  }
  auto clang_source_loc_id() const -> ClangSourceLocId {
    CARBON_CHECK(ir_id() == ImportIRId::Cpp);
    return clang_source_loc_id_;
  }

 private:
  ImportIRId ir_id_;
  union {
    // Set iff `ir_id != ImportIRId::Cpp`.
    InstId inst_id_;

    // Set iff `ir_id == ImportIRId::Cpp`.
    ClangSourceLocId clang_source_loc_id_;
  };
};

// Returns the canonical `File` and `InstId` for an entity, tracing imported
// instructions. Note the returned `File` might not be directly imported by the
// input `sem_ir`.
auto GetCanonicalFileAndInstId(const File* sem_ir, SemIR::InstId inst_id)
    -> std::pair<const File*, InstId>;

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_IMPORT_IR_H_
Repository: carbon-language/carbon-lang
File: common/build_data_linkstamp.h (C/C++)

Code before:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_
#define CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_

namespace Carbon::BuildData::Internal {

// See build_data.h; the list of names here should match.
//
// These are exposed as non-constexpr so that `build_data_linkstamp.cpp` can be
// compiled at the end, even though dependencies are added earlier.
//
// Also, when build_data_linkstamp.cpp is compiled, this doesn't receive deps,
// so we can't use things like `llvm::StringRef` here. As a result, we use
// `build_data.h` as an intermediary to do a `StringRef` wrap.
extern const char platform[];
extern const bool build_coverage_enabled;
extern const char target_name[];
extern const char build_target[];

}  // namespace Carbon::BuildData::Internal

#endif  // CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_


Review comment (suggestion):
I wasn't sure at first what this comment was trying to say, but I think I figured it out -- would this edit make sense to you? Or did I misunderstand?

```suggestion
// These are exposed as non-constexpr as they need to be initialized 
// in `build_data_linkstamp.cpp` that is separately compiled.
```

Code after:
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_
#define CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_

#include <string_view>

namespace Carbon::BuildData::Internal {

// See build_data.h; the list of names here should match.
//
// Bazel will build dependencies on the `:build_data` library (which exposes
// `build_data_linkstamp.h`) throughout the build process, but
// `build_data_linkstamp.cpp` is compiled and linked per-binary -- essentially a
// separate library. In essence, `build_data_linkstamp.h` is exposing values
// that are assigned later (this has consequences like preventing `constexpr`
// use).
//
// Also, when build_data_linkstamp.cpp is compiled, this doesn't receive deps,
// so we can't use things like `llvm::StringRef` here. As a result, we use
// `build_data.h` as an intermediary to do a `StringRef` wrap.
extern const std::string_view platform;
extern const bool build_coverage_enabled;
extern const std::string_view target_name;
extern const std::string_view build_target;

}  // namespace Carbon::BuildData::Internal

#endif  // CARBON_COMMON_BUILD_DATA_LINKSTAMP_H_