ronantakizawa/codereview-bench huggingface dataset:
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/specific_named_constraint.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_SPECIFIC_NAMED_CONSTRAINT_H_
#define CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_NAMED_CONSTRAINT_H_

#include "toolchain/base/canonical_value_store.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::SemIR {

// A pair of a named constraint and a specific for that named constraint.
struct SpecificNamedConstraint {
  static const SpecificNamedConstraint None;

  NamedConstraintId named_constraint_id;
  SpecificId specific_id;

  friend auto operator==(const SpecificNamedConstraint& lhs,
                         const SpecificNamedConstraint& rhs) -> bool = default;
};

constexpr SpecificNamedConstraint SpecificNamedConstraint::None = {
    .named_constraint_id = NamedConstraintId::None,
    .specific_id = SpecificId::None};

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_NAMED_CONSTRAINT_H_


Review comment (suggestion):
```suggestion
inline constexpr SpecificNamedConstraint SpecificNamedConstraint::None = {
```

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_SPECIFIC_NAMED_CONSTRAINT_H_
#define CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_NAMED_CONSTRAINT_H_

#include "toolchain/base/canonical_value_store.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::SemIR {

// A pair of a named constraint and a specific for that named constraint.
struct SpecificNamedConstraint {
  static const SpecificNamedConstraint None;

  NamedConstraintId named_constraint_id;
  SpecificId specific_id;

  friend auto operator==(const SpecificNamedConstraint& lhs,
                         const SpecificNamedConstraint& rhs) -> bool = default;
};

inline constexpr SpecificNamedConstraint SpecificNamedConstraint::None = {
    .named_constraint_id = NamedConstraintId::None,
    .specific_id = SpecificId::None};

}  // namespace Carbon::SemIR

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

Code before:
TEST(LatchTest, OnZeroCallback) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = handle;
  Latch::Handle handle3 = handle;

  EXPECT_FALSE(called);
  ASSERT_FALSE(std::move(handle).Drop());
  EXPECT_FALSE(called);
  ASSERT_FALSE(std::move(handle2).Drop());
  EXPECT_FALSE(called);
  ASSERT_TRUE(std::move(handle3).Drop());
  EXPECT_TRUE(called);
}

// Tests moving a handle.
TEST(LatchTest, MoveHandle) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = std::move(handle);

  // Check that dropping the new handle satisfies the latch.
  EXPECT_FALSE(called);
  std::move(handle2).Drop();
  EXPECT_TRUE(called);
}

// Test that creating and destroying a handle without dropping works.
TEST(LatchTest, Destructor) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  std::move(handle).Drop();
  EXPECT_TRUE(called);
}

// Tests the latch with multiple threads.
TEST(LatchTest, MultiThreaded) {
  Latch latch;
  std::atomic<int> counter = 0;
  bool called = false;
  constexpr int NumThreads = 5;


Review comment (suggestion):
```suggestion
  EXPECT_TRUE(std::move(handle2).Drop());
```

Code after:
TEST(LatchTest, OnZeroCallback) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = handle;
  Latch::Handle handle3 = handle;

  EXPECT_FALSE(called);
  EXPECT_FALSE(std::move(handle).Drop());
  EXPECT_FALSE(called);
  EXPECT_FALSE(std::move(handle2).Drop());
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle3).Drop());
  EXPECT_TRUE(called);
}

// Tests moving a handle.
TEST(LatchTest, MoveHandle) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = std::move(handle);

  // Check that dropping the new handle satisfies the latch.
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle2).Drop());
  EXPECT_TRUE(called);
}

// Test that creating and destroying a handle without dropping works.
TEST(LatchTest, Destructor) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle).Drop());
  EXPECT_TRUE(called);
}

// Tests calling `Init` more than once.
TEST(LatchTest, Reuse) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = handle;
Repository: carbon-language/carbon-lang
File: common/latch_test.cpp (C++)

Code before:

// Tests moving a handle.
TEST(LatchTest, MoveHandle) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = std::move(handle);

  // Check that dropping the new handle satisfies the latch.
  EXPECT_FALSE(called);
  std::move(handle2).Drop();
  EXPECT_TRUE(called);
}

// Test that creating and destroying a handle without dropping works.
TEST(LatchTest, Destructor) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  std::move(handle).Drop();
  EXPECT_TRUE(called);
}

// Tests the latch with multiple threads.
TEST(LatchTest, MultiThreaded) {
  Latch latch;
  std::atomic<int> counter = 0;
  bool called = false;
  constexpr int NumThreads = 5;

  // The `on_zero` callback will be executed by the last thread to drop its
  // handle.
  auto handle = latch.Init([&] {
    // Check that all threads have done their work.
    EXPECT_EQ(counter.load(), NumThreads);
    called = true;
  });

  std::deque<std::jthread> threads;
  for (int i = 0; i < NumThreads; ++i) {
    threads.emplace_back([&, handle_copy = handle] {
      // Each thread has its own copy of the handle.
      // Simulate some work.
      std::this_thread::sleep_for(std::chrono::milliseconds(10));
      counter++;

Review comment (suggestion):
```suggestion
  EXPECT_TRUE(std::move(handle).Drop());
```

Code after:

// Tests moving a handle.
TEST(LatchTest, MoveHandle) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = std::move(handle);

  // Check that dropping the new handle satisfies the latch.
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle2).Drop());
  EXPECT_TRUE(called);
}

// Test that creating and destroying a handle without dropping works.
TEST(LatchTest, Destructor) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle).Drop());
  EXPECT_TRUE(called);
}

// Tests calling `Init` more than once.
TEST(LatchTest, Reuse) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = handle;

  EXPECT_FALSE(called);
  EXPECT_FALSE(std::move(handle).Drop());
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle2).Drop());
  EXPECT_TRUE(called);

  // Now initialize the latch again with a new closure.
  bool called2 = false;
  Latch::Handle handle3 = latch.Init([&] { called2 = true; });
  Latch::Handle handle4 = handle3;

  EXPECT_FALSE(called2);
  EXPECT_FALSE(std::move(handle3).Drop());
  EXPECT_FALSE(called2);
  EXPECT_TRUE(std::move(handle4).Drop());
Repository: carbon-language/carbon-lang
File: common/latch_test.cpp (C++)

Code before:
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle).Drop());
  EXPECT_TRUE(called);
}

// Tests the latch with multiple threads.
TEST(LatchTest, MultiThreaded) {
  Latch latch;
  std::atomic<int> counter = 0;
  bool called = false;
  constexpr int NumThreads = 5;

  // The `on_zero` callback will be executed by the last thread to drop its
  // handle.
  auto handle = latch.Init([&] {
    // Check that all threads have done their work.
    EXPECT_EQ(counter.load(), NumThreads);
    called = true;
  });

  std::deque<std::thread> threads;
  for (int i = 0; i < NumThreads; ++i) {
    threads.emplace_back([&, handle_copy = handle] {
      // Each thread has its own copy of the handle.
      // Simulate some work.
      std::this_thread::sleep_for(std::chrono::milliseconds(10));
      counter++;
      // The handle is dropped when the thread exits.
    });
  }

  // Drop the main thread's handle.
  std::move(handle).Drop();

  for (auto& thread : threads) {
    thread.join();
  }
  EXPECT_TRUE(called);
}

}  // namespace
}  // namespace Carbon


Review comment (suggestion):
```suggestion
  std::vector<std::thread> threads;
```
Is there a specific reason for deque? It just kind of says "look at me" since it's not the default choice.

Code after:
  {
    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    Latch::Handle handle2 = handle;
    EXPECT_FALSE(called);
  }
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle).Drop());
  EXPECT_TRUE(called);
}

// Tests calling `Init` more than once.
TEST(LatchTest, Reuse) {
  Latch latch;
  bool called = false;
  Latch::Handle handle = latch.Init([&] { called = true; });
  Latch::Handle handle2 = handle;

  EXPECT_FALSE(called);
  EXPECT_FALSE(std::move(handle).Drop());
  EXPECT_FALSE(called);
  EXPECT_TRUE(std::move(handle2).Drop());
  EXPECT_TRUE(called);

  // Now initialize the latch again with a new closure.
  bool called2 = false;
  Latch::Handle handle3 = latch.Init([&] { called2 = true; });
  Latch::Handle handle4 = handle3;

  EXPECT_FALSE(called2);
  EXPECT_FALSE(std::move(handle3).Drop());
  EXPECT_FALSE(called2);
  EXPECT_TRUE(std::move(handle4).Drop());
  EXPECT_TRUE(called2);
}

// Tests the latch with multiple threads.
TEST(LatchTest, MultiThreaded) {
  Latch latch;
  std::atomic<int> counter = 0;
  bool called = false;
  constexpr int NumThreads = 5;

  // The `on_zero` callback will be executed by the last thread to drop its
  // handle.
  auto handle = latch.Init([&] {
    // Check that all threads have done their work.
    EXPECT_EQ(counter.load(), NumThreads);
    called = true;
  });

  std::vector<std::thread> threads;
Repository: carbon-language/carbon-lang
File: toolchain/check/cpp/macros.cpp (C++)

Code before:
  parser.ConsumeAnyToken(true);

  clang::ExprResult result = parser.ParseConstantExpression();
  clang::Expr* result_expr = result.get();

  bool success =
      !result.isInvalid() && parser.getCurToken().is(clang::tok::eof);

  if (!success) {
    parser.SkipUntil(clang::tok::eof);
    CARBON_DIAGNOSTIC(
        InCppMacroEvaluation, Error,
        "failed to evaluate macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  if (result_expr->EvaluateAsInt(evaluated_result, sema.getASTContext())) {
    return clang::IntegerLiteral::Create(
        sema.getASTContext(), evaluated_result.Val.getInt(),
        result_expr->getType(), result_expr->getExprLoc());
  }
  llvm::APFloat ap_value(0.0);
  if (result_expr->EvaluateAsFloat(ap_value, sema.getASTContext())) {
    return clang::FloatingLiteral::Create(
        sema.getASTContext(), ap_value,
        /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
  }

  context.TODO(loc_id, "Unsupported: constant type:" +
                           result_expr->getType().getAsString());
  return nullptr;
}

}  // namespace Carbon::Check


Review comment (suggestion):
Chatting with Richard, can you try `EvaluateAsConstantExpr` in place of both calls? i.e., maybe that would avoid individual calls here.

Code after:
                                /*IsReinject=*/false);
  parser.ConsumeAnyToken(true);

  clang::ExprResult result = parser.ParseConstantExpression();
  clang::Expr* result_expr = result.get();

  bool success =
      !result.isInvalid() && parser.getCurToken().is(clang::tok::eof);

  if (!success) {
    parser.SkipUntil(clang::tok::eof);
    CARBON_DIAGNOSTIC(
        InCppMacroEvaluation, Error,
        "failed to parse macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  CARBON_CHECK(result_expr->EvaluateAsConstantExpr(evaluated_result,
                                                   sema.getASTContext()));
  clang::APValue ap_value = evaluated_result.Val;
  switch (ap_value.getKind()) {
    case clang::APValue::Int:
      return clang::IntegerLiteral::Create(
          sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
          result_expr->getExprLoc());
    case clang::APValue::Float:
      return clang::FloatingLiteral::Create(
          sema.getASTContext(), ap_value.getFloat(),
          /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
    default:
      context.TODO(loc_id,
                   "Unsupported: macro evaluated to a constant of type: " +
                       result_expr->getType().getAsString());
      return nullptr;
  }
}

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

Code before:
        InCppMacroEvaluation, Error,
        "failed to parse macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  if (!result_expr->EvaluateAsConstantExpr(evaluated_result,
                                           sema.getASTContext())) {
    context.TODO(loc_id,
                 "failed to evaluate macro to a valid constant expression");
    return nullptr;
  }
  clang::APValue ap_value = evaluated_result.Val;
  if (ap_value.isInt()) {
    return clang::IntegerLiteral::Create(
        sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
        result_expr->getExprLoc());
  }
  if (ap_value.isFloat()) {
    return clang::FloatingLiteral::Create(
        sema.getASTContext(), ap_value.getFloat(),
        /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
  }
  context.TODO(loc_id, "Unsupported: constant type:" +
                           result_expr->getType().getAsString());
  return nullptr;
}

}  // namespace Carbon::Check


Review comment (suggestion):
```suggestion
  context.TODO(loc_id, "Unsupported: constant type: " +
```

Very minor nit, would've just merged if I didn't have the other comment

Code after:
        InCppMacroEvaluation, Error,
        "failed to parse macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  CARBON_CHECK(result_expr->EvaluateAsConstantExpr(evaluated_result,
                                                   sema.getASTContext()));
  clang::APValue ap_value = evaluated_result.Val;
  switch (ap_value.getKind()) {
    case clang::APValue::Int:
      return clang::IntegerLiteral::Create(
          sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
          result_expr->getExprLoc());
    case clang::APValue::Float:
      return clang::FloatingLiteral::Create(
          sema.getASTContext(), ap_value.getFloat(),
          /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
    default:
      context.TODO(loc_id,
                   "Unsupported: macro evaluated to a constant of type: " +
                       result_expr->getType().getAsString());
      return nullptr;
  }
}

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

Code before:

  clang::ExprResult result = parser.ParseConstantExpression();
  clang::Expr* result_expr = result.get();

  bool success =
      !result.isInvalid() && parser.getCurToken().is(clang::tok::eof);

  if (!success) {
    parser.SkipUntil(clang::tok::eof);
    CARBON_DIAGNOSTIC(
        InCppMacroEvaluation, Error,
        "failed to parse macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  if (!result_expr->EvaluateAsConstantExpr(evaluated_result,
                                           sema.getASTContext())) {
    context.TODO(loc_id,
                 "failed to evaluate macro to a valid constant expression");
    return nullptr;
  }
  clang::APValue ap_value = evaluated_result.Val;
  if (ap_value.isInt()) {
    return clang::IntegerLiteral::Create(
        sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
        result_expr->getExprLoc());
  }
  if (ap_value.isFloat()) {
    return clang::FloatingLiteral::Create(
        sema.getASTContext(), ap_value.getFloat(),
        /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
  }
  context.TODO(loc_id, "Unsupported: constant type:" +
                           result_expr->getType().getAsString());
  return nullptr;
}

}  // namespace Carbon::Check


Review comment (suggestion):
Had you considered switching on `getKind`? Might make it a little easier to add more types to this.

Code after:

  clang::ExprResult result = parser.ParseConstantExpression();
  clang::Expr* result_expr = result.get();

  bool success =
      !result.isInvalid() && parser.getCurToken().is(clang::tok::eof);

  if (!success) {
    parser.SkipUntil(clang::tok::eof);
    CARBON_DIAGNOSTIC(
        InCppMacroEvaluation, Error,
        "failed to parse macro Cpp.{0} to a valid constant expression",
        std::string);
    context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
    return nullptr;
  }

  clang::Expr::EvalResult evaluated_result;
  CARBON_CHECK(result_expr->EvaluateAsConstantExpr(evaluated_result,
                                                   sema.getASTContext()));
  clang::APValue ap_value = evaluated_result.Val;
  switch (ap_value.getKind()) {
    case clang::APValue::Int:
      return clang::IntegerLiteral::Create(
          sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
          result_expr->getExprLoc());
    case clang::APValue::Float:
      return clang::FloatingLiteral::Create(
          sema.getASTContext(), ap_value.getFloat(),
          /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
    default:
      context.TODO(loc_id,
                   "Unsupported: macro evaluated to a constant of type: " +
                       result_expr->getType().getAsString());
      return nullptr;
  }
}

}  // namespace Carbon::Check
Repository: carbon-language/carbon-lang
File: scripts/create_compdb.py (Python)

Code before:
        # Use indent=4 for a human-readable, pretty-printed output file
        json.dump(filtered_commands, f, indent=4)
    print(
        "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",

Review comment (suggestion):
nit: you specify this each time with one flag
```suggestion
        "--extra-bazel-flag",
```

Code after:
        # Use indent=4 for a human-readable, pretty-printed output file
        json.dump(filtered_commands, f, indent=4)
    print(
        "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",
        ]