ronantakizawa/codereview-bench huggingface dataset:
Repository: carbon-language/carbon-lang
File: scripts/create_compdb.py (Python)

Code before:
        "Filtered out "
        f"{len(commands) - len(filtered_commands)} "
        "duplicate entries..."
    )


def main() -> None:
    parser = argparse.ArgumentParser(
        description=__doc__,
        allow_abbrev=False,
    )
    parser.add_argument(
        "--alsologtostderr",
        action="store_true",
        help="Prints subcommand errors to stderr (default: False)",
    )
    parser.add_argument(
        "--dump-files",
        action="store_true",
        help="Dumps the full list of generated files (default: False)",
    )
    parser.add_argument(
        "--extra-bazel-flags",
        action="append",
        default=[],
        help="Extra flags to pass to Bazel invocations",
    )

    args = parser.parse_args()
    scripts_utils.chdir_repo_root()
    bazel = scripts_utils.locate_bazel()

    _build_generated_files(
        bazel, args.alsologtostderr, args.dump_files, args.extra_bazel_flags
    )

    print(
        "Generating compile_commands.json (may take a few minutes)...",
        flush=True,
    )
    subprocess.run(
        [
            bazel,
            "run",
        ]
        + args.extra_bazel_flags
        + [
            "@hedron_compile_commands//:refresh_all",
            "--",
        ]
        + args.extra_bazel_flags

Review comment (suggestion):
```suggestion
        help="Extra flag to pass to Bazel invocations, may be specified more than once",
```

Code after:
        "Filtered out "
        f"{len(commands) - len(filtered_commands)} "
        "duplicate entries..."
    )


def main() -> None:
    parser = argparse.ArgumentParser(
        description=__doc__,
        allow_abbrev=False,
    )
    parser.add_argument(
        "--alsologtostderr",
        action="store_true",
        help="Prints subcommand errors to stderr (default: False)",
    )
    parser.add_argument(
        "--dump-files",
        action="store_true",
        help="Dumps the full list of generated files (default: False)",
    )
    parser.add_argument(
        "--extra-bazel-flag",
        action="append",
        default=[],
        help=(
            "Extra flag to pass to Bazel invocations, may be specified more "
            "than once"
        ),
    )

    args = parser.parse_args()
    scripts_utils.chdir_repo_root()
    bazel = scripts_utils.locate_bazel()

    _build_generated_files(
        bazel, args.alsologtostderr, args.dump_files, args.extra_bazel_flag
    )

    print(
        "Generating compile_commands.json (may take a few minutes)...",
        flush=True,
    )
    subprocess.run(
        [
            bazel,
            "run",
        ]
        + args.extra_bazel_flag
        + [
            "@hedron_compile_commands//:refresh_all",
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/builtin_function_kind.cpp (C++)

Code before:
// simplicity, but should probably be revisited.
static auto AnyLiteralTypes(const File& sem_ir, llvm::ArrayRef<InstId> arg_ids,
                            TypeId return_type_id) -> bool {
  if (IsLiteralType(sem_ir, return_type_id)) {
    return true;
  }
  for (auto arg_id : arg_ids) {
    if (IsLiteralType(sem_ir, sem_ir.insts().Get(arg_id).type_id())) {
      return true;
    }
  }
  return false;
}

auto BuiltinFunctionKind::IsCompTimeOnly(const File& sem_ir,
                                         llvm::ArrayRef<InstId> arg_ids,
                                         TypeId return_type_id) const -> bool {
  switch (*this) {
    case CharConvertChecked:
    case FloatConvertChecked:
    case IntConvertChecked:
      // Checked conversions are compile-time only.
      return true;

    case IntConvertChar:
    case IntConvert:
    case IntSNegate:
    case IntComplement:
    case IntSAdd:
    case IntSSub:
    case IntSMul:
    case IntSDiv:
    case IntSMod:
    case IntAnd:
    case IntOr:
    case IntXor:
    case IntLeftShift:
    case IntRightShift:
    case IntEq:
    case IntNeq:
    case IntLess:
    case IntLessEq:
    case IntGreater:
    case IntGreaterEq:
      // Integer operations are compile-time-only if they involve literal types.
      // See AnyLiteralTypes comment for explanation.
      return AnyLiteralTypes(sem_ir, arg_ids, return_type_id);

    case TypeAnd:
      return true;


Review comment (suggestion):
```suggestion
    case IntConvert:
    case IntConvertChar:
```
Prefixes sort first

Code after:
static auto AnyLiteralTypes(const File& sem_ir, llvm::ArrayRef<InstId> arg_ids,
                            TypeId return_type_id) -> bool {
  if (IsLiteralType(sem_ir, return_type_id)) {
    return true;
  }
  for (auto arg_id : arg_ids) {
    if (IsLiteralType(sem_ir, sem_ir.insts().Get(arg_id).type_id())) {
      return true;
    }
  }
  return false;
}

auto BuiltinFunctionKind::IsCompTimeOnly(const File& sem_ir,
                                         llvm::ArrayRef<InstId> arg_ids,
                                         TypeId return_type_id) const -> bool {
  switch (*this) {
    case CharConvertChecked:
    case FloatConvertChecked:
    case IntConvertChecked:
      // Checked conversions are compile-time only.
      return true;

    case IntConvert:
    case IntConvertChar:
    case IntSNegate:
    case IntComplement:
    case IntSAdd:
    case IntSSub:
    case IntSMul:
    case IntSDiv:
    case IntSMod:
    case IntAnd:
    case IntOr:
    case IntXor:
    case IntLeftShift:
    case IntRightShift:
    case IntEq:
    case IntNeq:
    case IntLess:
    case IntLessEq:
    case IntGreater:
    case IntGreaterEq:
      // Integer operations are compile-time-only if they involve literal types.
      // See AnyLiteralTypes comment for explanation.
      return AnyLiteralTypes(sem_ir, arg_ids, return_type_id);

    case TypeAnd:
      return true;

    case TypeCanDestroy:
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/cpp_file.h (C/C++)

Code before:
    return ast_unit_->getLangOpts();
  }

  // Access to Clang's compilation environment.
  auto source_manager() -> clang::SourceManager& {
    return ast_unit_->getSourceManager();
  }
  auto source_manager() const -> const clang::SourceManager& {
    return ast_unit_->getSourceManager();
  }
  auto diagnostics() const -> clang::DiagnosticsEngine& {
    return ast_unit_->getDiagnostics();
  }

  // Access to layers of Clang's C++ representation.
  auto ast_context() -> clang::ASTContext& {
    return ast_unit_->getASTContext();
  }
  auto ast_context() const -> const clang::ASTContext& {
    return ast_unit_->getASTContext();
  }
  auto sema() -> clang::Sema& { return ast_unit_->getSema(); }

  // Visit all top-level declarations in the file.
  auto VisitLocalTopLevelDecls(
      llvm::function_ref<void(const clang::Decl*)> visitor) const -> void;

 private:
  std::unique_ptr<clang::ASTUnit> ast_unit_;
};

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_CPP_FILE_H_


Review comment (suggestion):
```suggestion
      llvm::function_ref<auto(const clang::Decl*)->void> visitor) const -> void;
```
Trailing return nit

Code after:
    return ast_unit_->getLangOpts();
  }

  // Access to Clang's compilation environment.
  auto source_manager() -> clang::SourceManager& {
    return ast_unit_->getSourceManager();
  }
  auto source_manager() const -> const clang::SourceManager& {
    return ast_unit_->getSourceManager();
  }
  auto diagnostics() const -> clang::DiagnosticsEngine& {
    return ast_unit_->getDiagnostics();
  }

  // Access to layers of Clang's C++ representation.
  auto ast_context() -> clang::ASTContext& {
    return ast_unit_->getASTContext();
  }
  auto ast_context() const -> const clang::ASTContext& {
    return ast_unit_->getASTContext();
  }
  auto sema() -> clang::Sema& { return ast_unit_->getSema(); }

  // Visit all top-level declarations in the file.
  auto VisitLocalTopLevelDecls(
      llvm::function_ref<auto(const clang::Decl*)->void> visitor) const -> void;

 private:
  std::unique_ptr<clang::ASTUnit> ast_unit_;
};

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_CPP_FILE_H_
Repository: carbon-language/carbon-lang
File: common/string_helpers_test.cpp (C++)

Code before:
  llvm::OwningArrayRef<char> storage;
  std::string prefix_args[] = {"p_arg1", "p_arg2"};
  auto result = BuildCStrArgs("tool", prefix_args, {}, storage);
  ASSERT_THAT(result.size(), Eq(3));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], StrEq("p_arg1"));
  EXPECT_THAT(result[2], StrEq("p_arg2"));
}

TEST(BuildCStrArgsWithPrefix, ArgsOnly) {
  llvm::OwningArrayRef<char> storage;
  auto result = BuildCStrArgs("tool", {}, {"arg1", "arg2"}, storage);
  ASSERT_THAT(result.size(), Eq(3));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], StrEq("arg1"));
  EXPECT_THAT(result[2], StrEq("arg2"));
}

TEST(BuildCStrArgsWithPrefix, BothPrefixAndArgs) {
  llvm::OwningArrayRef<char> storage;
  std::string prefix_args[] = {"p_arg1", "p_arg2"};
  auto result = BuildCStrArgs("tool", prefix_args, {"arg1", "arg2"}, storage);
  ASSERT_THAT(result.size(), Eq(5));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], StrEq("p_arg1"));
  EXPECT_THAT(result[2], StrEq("p_arg2"));
  EXPECT_THAT(result[3], StrEq("arg1"));
  EXPECT_THAT(result[4], StrEq("arg2"));
}

}  // namespace
}  // namespace Carbon


Review comment (suggestion):
As above, maybe this instead?

```suggestion
  EXPECT_EQ(result[1], prefix_args[0].c_str());
  EXPECT_EQ(result[2], prefix_args[1].c_str());
```


Code after:
  llvm::OwningArrayRef<char> storage;
  std::string prefix_args[] = {"p_arg1", "p_arg2"};
  auto result = BuildCStrArgs("tool", prefix_args, {}, storage);
  ASSERT_THAT(result.size(), Eq(3));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], Eq(prefix_args[0].c_str()));
  EXPECT_THAT(result[2], Eq(prefix_args[1].c_str()));
}

TEST(BuildCStrArgsWithPrefix, ArgsOnly) {
  llvm::OwningArrayRef<char> storage;
  auto result = BuildCStrArgs("tool", {}, {"arg1", "arg2"}, storage);
  ASSERT_THAT(result.size(), Eq(3));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], StrEq("arg1"));
  EXPECT_THAT(result[2], StrEq("arg2"));
}

TEST(BuildCStrArgsWithPrefix, BothPrefixAndArgs) {
  llvm::OwningArrayRef<char> storage;
  std::string prefix_args[] = {"p_arg1", "p_arg2"};
  auto result = BuildCStrArgs("tool", prefix_args, {"arg1", "arg2"}, storage);
  ASSERT_THAT(result.size(), Eq(5));
  EXPECT_THAT(result[0], StrEq("tool"));
  EXPECT_THAT(result[1], Eq(prefix_args[0].c_str()));
  EXPECT_THAT(result[2], Eq(prefix_args[1].c_str()));
  EXPECT_THAT(result[3], StrEq("arg1"));
  EXPECT_THAT(result[4], StrEq("arg2"));
}

}  // namespace
}  // namespace Carbon
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/entry_point.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 "toolchain/sem_ir/entry_point.h"

#include "llvm/ADT/StringRef.h"

namespace Carbon::SemIR {

static constexpr llvm::StringLiteral EntryPointFunction = "Run";

auto IsEntryPoint(const File& file, FunctionId function_id) -> bool {
  // TODO: Check if `file` is in the `Main` package.
  const auto& function = file.functions().Get(function_id);

  if (function.clang_decl_id.has_value()) {
    return false;
  }

  // TODO: Check if `function` is in a namespace.
  return function.name_id.has_value() &&
         file.names().GetAsStringIfIdentifier(function.name_id) ==
             EntryPointFunction;
}

}  // namespace Carbon::SemIR


Review comment (bug):
It looks like we'd also have the same bug if a Carbon function named `Run` appeared somewhere else -- in a package other than `Main` or in a nested scope such as within a namespace. I think we could catch that issue too by checking two things:

1) `function.name_scope_id == NameScopeId::Package` -- the function must be at the top level of the package, and
2) the package must be the main package -- `file.package_id() == PackageNameId::None`

With those checks we shouldn't need the explicit C++ check.

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 "toolchain/sem_ir/entry_point.h"

#include "llvm/ADT/StringRef.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::SemIR {

static constexpr llvm::StringLiteral EntryPointFunction = "Run";

auto IsEntryPoint(const File& file, FunctionId function_id) -> bool {
  const auto& function = file.functions().Get(function_id);

  return function.parent_scope_id == NameScopeId::Package &&
         file.package_id() == PackageNameId::None &&
         function.name_id.has_value() &&
         file.names().GetAsStringIfIdentifier(function.name_id) ==
             EntryPointFunction;
}

}  // namespace Carbon::SemIR
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/formatter_chunks.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_TOOLCHAIN_SEM_IR_FORMATTER_CHUNKS_H_
#define CARBON_TOOLCHAIN_SEM_IR_FORMATTER_CHUNKS_H_

#include <string>

#include "common/check.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/raw_ostream.h"

namespace Carbon::SemIR {

// Manages the chunks created by the formatter.
//
// There are two kinds of `Chunk`s:
// - Parent `Chunk`s, with children in a vector.
// - Content `Chunk`s, with content in a string.
//
// Initially, `AddParent` is called one or more times. Then `StartContent` is
// called once to switch modes. Once content is started, `out()` adds text to
// content `Chunk`s. `FormatChildContent` may be used to create a content
// `Chunk` which is a child of a parent.
//
// Content `Chunk`s that are created implicitly (not through
// `FormatChildContent`) are automatically included in output. Other `Chunk`s
// are included only when marked as a child of a `Chunk` that is included, using
// `AppendChildToCurrentParent`.
class FormatterChunks {
 public:
  // A type-safe index into `chunks_`.
  struct ChunkId {
    auto operator==(const ChunkId& other) const -> bool = default;

    size_t index;
  };

  // An empty `ChunkId`.
  static constexpr ChunkId None = ChunkId(-1);

  // Reserves space for at least `count` chunks.

Review comment (suggestion):
Now that `Chunk` is private, I'd suggest replacing "`Chunk`" with "chunk" throughout the header comments.

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_TOOLCHAIN_SEM_IR_FORMATTER_CHUNKS_H_
#define CARBON_TOOLCHAIN_SEM_IR_FORMATTER_CHUNKS_H_

#include <string>

#include "common/check.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/raw_ostream.h"

namespace Carbon::SemIR {

// Manages the chunks created by the formatter.
//
// There are two kinds of `Chunk`s:
// - Parent `Chunk`s, with children in a vector.
// - Content `Chunk`s, with content in a string.
//
// The high level usage is:
// 1. Calls to `AddParent` to prepare parent `Chunk`s.
// 2. One call to `StartContent` to switch modes.
// 3. Calls to `FormatChildContent`, `AppendChildToCurrentParent`, and `out`.
// 4. Calls to `Write`.
class FormatterChunks {
 public:
  // A type-safe index into `chunks_`.
  struct ChunkId {
    auto operator==(const ChunkId& other) const -> bool = default;

    size_t index;
  };

  // An empty `ChunkId`.
  static constexpr ChunkId None = ChunkId(-1);

  // Reserves space for at least `count` chunks.
  auto Reserve(size_t count) -> void { chunks_.reserve(count); }

  // Adds a parent `Chunk` and returns its `ChunkId`. If `child_chunk_id` isn't
  // `None`, it's added as a child. Must be called before `StartContent`.
Repository: casey/just
File: src/function.rs (Rust)

Code before:
}

fn file_name(_context: &FunctionContext, file_path: &str) -> Result<String, String> {
  Utf8Path::new(file_path)
    .file_name()
    .map(std::borrow::ToOwned::to_owned)
    .ok_or_else(|| format!("Cannot get file name from `{}`", file_path))
}

fn parent_directory(_context: &FunctionContext, file_path: &str) -> Result<String, String> {
  Utf8Path::new(file_path)
    .parent()
    .map(Utf8Path::as_str)
    .map(std::borrow::ToOwned::to_owned)
    .ok_or_else(|| format!("Cannot get parent directory from `{}`", file_path))
}

fn file_stem(_context: &FunctionContext, file_path: &str) -> Result<String, String> {
  Utf8Path::new(file_path)
    .file_stem()
    .map(std::borrow::ToOwned::to_owned)
    .ok_or_else(|| format!("Cannot get file stem from `{}`", file_path))
}

fn without_extension(_context: &FunctionContext, file_path: &str) -> Result<String, String> {
  let path = Utf8Path::new(file_path);
  path
    .parent()
    .map(Utf8Path::as_str)
    .zip(path.file_stem())
    .map(|(p, n)| format!("{}/{}", p, n))
    .ok_or_else(|| format!("Cannot remove extension from `{}`", file_path))
}

fn extension(_context: &FunctionContext, file_path: &str) -> Result<String, String> {
  Utf8Path::new(file_path)
    .extension()
    .map(std::borrow::ToOwned::to_owned)
    .ok_or_else(|| format!("Cannot get extension from `{}`", file_path))
}


Review comment (suggestion):
I think it would be better to do this in two steps, first get the parent and report an error if you can't get it, then get the file_stem and report an error, and then return the error.

Also, you can do `Utf8Path::join` instead of `format!("{}/{}", …)`.

Code after:

fn file_name(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_name()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract file name from `{}`", path))
}

fn parent_directory(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{}`", path))
}

fn file_stem(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_stem()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract file stem from `{}`", path))
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{}`", path))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{}`", path))?;

  Ok(parent.join(file_stem).to_string())
}

fn extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .extension()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract extension from `{}`", path))
}
Repository: casey/just
File: src/function.rs (Rust)

Code before:
    format!(
      "Executable path is not valid unicode: {}",
      exe_path.to_string_lossy()
    )
  })
}

fn file_name(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_name()
    .map(str::to_owned)
    .ok_or_else(|| format!("Cannot get file name from `{}`", path))
}

fn parent_directory(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Cannot get parent directory from `{}`", path))
}

fn file_stem(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_stem()
    .map(str::to_owned)
    .ok_or_else(|| format!("Cannot get file stem from `{}`", path))
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Cannot extract parent from `{}`", path))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Cannot extract file stem from `{}`", path))?;

  Ok(parent.join(file_stem).to_string())
}

fn extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .extension()
    .map(str::to_owned)
    .ok_or_else(|| format!("Cannot get extension from `{}`", path))
}


Review comment (refactor):
```suggestion
    .ok_or_else(|| format!("Could not extract file stem from `{}`", path))
```

Code after:
    format!(
      "Executable path is not valid unicode: {}",
      exe_path.to_string_lossy()
    )
  })
}

fn file_name(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_name()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract file name from `{}`", path))
}

fn parent_directory(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .parent()
    .map(Utf8Path::to_string)
    .ok_or_else(|| format!("Could not extract parent directory from `{}`", path))
}

fn file_stem(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .file_stem()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract file stem from `{}`", path))
}

fn without_extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  let parent = Utf8Path::new(path)
    .parent()
    .ok_or_else(|| format!("Could not extract parent from `{}`", path))?;

  let file_stem = Utf8Path::new(path)
    .file_stem()
    .ok_or_else(|| format!("Could not extract file stem from `{}`", path))?;

  Ok(parent.join(file_stem).to_string())
}

fn extension(_context: &FunctionContext, path: &str) -> Result<String, String> {
  Utf8Path::new(path)
    .extension()
    .map(str::to_owned)
    .ok_or_else(|| format!("Could not extract extension from `{}`", path))
}