ronantakizawa/codereview-bench huggingface dataset:
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):
Could we use `std::string_view` for these? That would let us compute the size in the `.cpp` file, and access that size when initializing our `llvm::StringRef` public variables. I think that would avoid needing to do `strlen` to compute the size in a dynamic initializer.

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_
Repository: carbon-language/carbon-lang
File: common/build_data.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_H_
#define CARBON_COMMON_BUILD_DATA_H_

#include "common/build_data_linkstamp.h"
#include "llvm/ADT/StringRef.h"

namespace Carbon::BuildData {

// Build information for a binary, from bazel. Stamped values come from:
// https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java

// NOLINTBEGIN(readability-identifier-naming): We want to use constant-style
// names for the public variables, but cannot use constexpr.

// The platform, per https://bazel.build/extending/platforms.
static const llvm::StringRef Platform = Internal::platform;

// Whether coverage is enabled.
static const bool BuildCoverageEnabled = Internal::build_coverage_enabled;

// The binary target, such as `//common:build_data_test`.
static const llvm::StringRef TargetName = Internal::target_name;

// The path to the build target, such as
// `bazel-out/k8-fastbuild/bin/common/build_data_test`.
static const llvm::StringRef BuildTarget = Internal::build_target;

// NOLINTEND(readability-identifier-naming)

}  // namespace Carbon::BuildData

#endif  // CARBON_COMMON_BUILD_DATA_H_


Review comment (suggestion):
I think these need to be `inline` instead of `static`. That should let us define and initialize them in the header while letting them be merged between translation units.

Can you use `llvm::StringLiteral` here even without the `constexpr`? Not sure myself, could see it going either way. And a minor point.

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_H_
#define CARBON_COMMON_BUILD_DATA_H_

#include "common/build_data_linkstamp.h"
#include "llvm/ADT/StringRef.h"

namespace Carbon::BuildData {

// Build information for a binary, from bazel. Stamped values come from:
// https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java

// NOLINTBEGIN(readability-identifier-naming): We want to use constant-style
// names for the public variables, but cannot use constexpr.

// The platform, per https://bazel.build/extending/platforms.
inline const llvm::StringRef Platform = Internal::platform;

// Whether coverage is enabled.
inline const bool BuildCoverageEnabled = Internal::build_coverage_enabled;

// The binary target, such as `//common:build_data_test`.
inline const llvm::StringRef TargetName = Internal::target_name;

// The path to the build target, such as
// `bazel-out/k8-fastbuild/bin/common/build_data_test`.
inline const llvm::StringRef BuildTarget = Internal::build_target;

// NOLINTEND(readability-identifier-naming)

}  // namespace Carbon::BuildData

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

Code before:
        } else {
          add_inst_name("tuple");
        }
        continue;
      }
      case CARBON_KIND(UnboundElementType inst): {
        if (auto class_ty =
                sem_ir_->insts().TryGetAs<ClassType>(inst.class_type_inst_id)) {
          add_inst_name_id(sem_ir_->classes().Get(class_ty->class_id).name_id,
                           ".elem");
        } else {
          add_inst_name("elem_type");
        }
        continue;
      }
      case CARBON_KIND(VarStorage inst): {
        add_inst_name_id(inst.pretty_name_id, ".var");
        continue;
      }
      default: {
        // Sequentially number all remaining values.
        if (untyped_inst.kind().has_type()) {
          add_inst_name("");
        }
      }
    }
  }
}

auto InstNamer::CollectNamesInGeneric(ScopeId scope_id, GenericId generic_id)
    -> void {
  if (!generic_id.has_value()) {
    return;
  }
  generic_scopes_[generic_id.index] = scope_id;
  const auto& generic = sem_ir_->generics().Get(generic_id);
  CollectNamesInBlock(scope_id, generic.decl_block_id);
  CollectNamesInBlock(scope_id, generic.definition_block_id);
}

}  // namespace Carbon::SemIR


Review comment (suggestion):
Please add a `break` (or `continue`, if you keep all the `continue`s) here too, rather than falling off the bottom of the switch.

Code after:
        } else {
          add_inst_name("tuple");
        }
        continue;
      }
      case CARBON_KIND(UnboundElementType inst): {
        if (auto class_ty =
                sem_ir_->insts().TryGetAs<ClassType>(inst.class_type_inst_id)) {
          add_inst_name_id(sem_ir_->classes().Get(class_ty->class_id).name_id,
                           ".elem");
        } else {
          add_inst_name("elem_type");
        }
        continue;
      }
      case CARBON_KIND(VarStorage inst): {
        add_inst_name_id(inst.pretty_name_id, ".var");
        continue;
      }
      default: {
        // Sequentially number all remaining values.
        if (untyped_inst.kind().has_type()) {
          add_inst_name("");
        }
        continue;
      }
    }
  }
}

auto InstNamer::CollectNamesInGeneric(ScopeId scope_id, GenericId generic_id)
    -> void {
  if (!generic_id.has_value()) {
    return;
  }
  generic_scopes_[generic_id.index] = scope_id;
  const auto& generic = sem_ir_->generics().Get(generic_id);
  CollectNamesInBlock(scope_id, generic.decl_block_id);
  CollectNamesInBlock(scope_id, generic.definition_block_id);
}

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

Code before:
  auto Get(IdT id) const -> ConstRefType {
    CARBON_DCHECK(id.index >= 0, "{0}", id);
    return values_[id.index];
  }

  // Collects memory usage of the values.
  auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const
      -> void {
    mem_usage.Collect(label.str(), values_);
  }

  auto size() const -> size_t { return values_.size(); }
  auto values() -> auto {
    return llvm::make_range(values_.begin(), values_.end());
  }

 private:
  // Allow default construction for `Make` functions.
  FixedSizeValueStore() = default;

  // Storage for the `ValueT` objects, indexed by the id.
  llvm::SmallVector<ValueT, 0> values_;
};

}  // namespace Carbon

#endif  // CARBON_TOOLCHAIN_BASE_FIXED_SIZE_VALUE_STORE_H_


Review comment (bug):
Not for this PR I guess, as we have a pre-existing one, but given the unsafe ctor methods, WDYT of keeping a second SmallVector in non-NDEBUG builds that tracks which elements were initialized, and CHECKs that we don't Get something uninitialized?

Code after:
    CARBON_DCHECK(id.index >= 0, "{0}", id);
    return values_[id.index];
  }

  // Returns the value for an ID.
  auto Get(IdT id) const -> ConstRefType {
    CARBON_DCHECK(id.index >= 0, "{0}", id);
    return values_[id.index];
  }

  // Collects memory usage of the values.
  auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const
      -> void {
    mem_usage.Collect(label.str(), values_);
  }

  auto size() const -> size_t { return values_.size(); }
  auto values() -> auto {
    return llvm::make_range(values_.begin(), values_.end());
  }

 private:
  // Allow default construction for `Make` functions.
  FixedSizeValueStore() = default;

  // Storage for the `ValueT` objects, indexed by the id.
  llvm::SmallVector<ValueT, 0> values_;
};

}  // namespace Carbon

#endif  // CARBON_TOOLCHAIN_BASE_FIXED_SIZE_VALUE_STORE_H_
Repository: carbon-language/carbon-lang
File: common/exe_path.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_EXE_PATH_H_
#define CARBON_COMMON_EXE_PATH_H_

#include "llvm/ADT/StringRef.h"

namespace Carbon {

// Computes the executable path for the given `argv[0]` value form `main`.
// `argv0` is required to be null-terminated.
//
// A simplistic approach -- if the provided string isn't already a valid path,
// we look it up in the PATH environment variable. Doesn't resolve any symlinks
// and if it fails, simply returns the provided `argv0`.
auto FindExecutablePath(const char* argv0) -> std::string;

}  // namespace Carbon

#endif  // CARBON_COMMON_EXE_PATH_H_


Review comment (suggestion):
```suggestion
// Computes the executable path for the given `argv[0]` value from `main`.
```
(drive-by, but I was having real trouble understanding what this meant)

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_EXE_PATH_H_
#define CARBON_COMMON_EXE_PATH_H_

#include "llvm/ADT/StringRef.h"

namespace Carbon {

// Computes the executable path for the given `argv[0]` value from `main`.
// `argv0` is required to be null-terminated.
//
// A simplistic approach -- if the provided string isn't already a valid path,
// we look it up in the PATH environment variable. Doesn't resolve any symlinks
// and if it fails, returns the main executable path.
auto FindExecutablePath(const char* argv0) -> std::string;

}  // namespace Carbon

#endif  // CARBON_COMMON_EXE_PATH_H_
Repository: carbon-language/carbon-lang
File: toolchain/install/busybox_info.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_INSTALL_BUSYBOX_INFO_H_
#define CARBON_TOOLCHAIN_INSTALL_BUSYBOX_INFO_H_

#include <filesystem>
#include <optional>
#include <string>

#include "common/error.h"

namespace Carbon {

// An optional override of argv0, particularly used by `//toolchain/carbon` to
// get desired behavior without further special-casing.
constexpr const char* Argv0OverrideEnv = "CARBON_ARGV0_OVERRIDE";

struct BusyboxInfo {
  // The path to `carbon-busybox`.
  std::filesystem::path bin_path;
  // The mode, such as `carbon` or `clang`.
  std::optional<std::string> mode;
};

// Returns the busybox information, given argv[0].
//
// Extracts the desired mode for the busybox from the initial command name.
//
// Checks if the path in argv0 is an executable in a valid Carbon install, or a
// symlink to such an executable, and sets `bin_path` to the path of
// `lib/carbon/carbon-busybox` within that install.
//
// If unable to locate a plausible busybox binary, returns an error instead.
auto GetBusyboxInfo(const char* argv0) -> ErrorOr<BusyboxInfo>;

}  // namespace Carbon

#endif  // CARBON_TOOLCHAIN_INSTALL_BUSYBOX_INFO_H_


Review comment (suggestion):
While we're factoring stuff out: this should either be `inline` or have its definition moved to the `.cpp` file too, to avoid ODR issues.
```suggestion
inline constexpr const char* Argv0OverrideEnv = "CARBON_ARGV0_OVERRIDE";
```

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_INSTALL_BUSYBOX_INFO_H_
#define CARBON_TOOLCHAIN_INSTALL_BUSYBOX_INFO_H_

#include <filesystem>
#include <optional>
#include <string>

#include "common/error.h"

namespace Carbon {

// An optional override of argv0, particularly used by `//toolchain/carbon` to
// get desired behavior without further special-casing.
inline constexpr const char* Argv0OverrideEnv = "CARBON_ARGV0_OVERRIDE";

struct BusyboxInfo {
  // The path to `carbon-busybox`.
  std::filesystem::path bin_path;
  // The mode, such as `carbon` or `clang`.
  std::optional<std::string> mode;
};

// Returns the busybox information, given argv[0].
//
// Extracts the desired mode for the busybox from the initial command name.
//
// Checks if the path in argv0 is an executable in a valid Carbon install, or a
// symlink to such an executable, and sets `bin_path` to the path of
// `lib/carbon/carbon-busybox` within that install.
//
// If unable to locate a plausible busybox binary, returns an error instead.
auto GetBusyboxInfo(const char* argv0) -> ErrorOr<BusyboxInfo>;

}  // namespace Carbon

#endif  // CARBON_TOOLCHAIN_INSTALL_BUSYBOX_INFO_H_
Repository: carbon-language/carbon-lang
File: scripts/check_build_graph.py (Python)

Code before:
#!/usr/bin/env python3

"""Verify that the bazel build graph is in a valid state, for pre-commit."""

__copyright__ = """
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
"""

import subprocess
import sys

import scripts_utils


def main() -> int:
    scripts_utils.chdir_repo_root()
    bazel = scripts_utils.locate_bazel()
    sys.exit(subprocess.run([bazel, "build", "--nobuild", "//..."]).returncode)


if __name__ == "__main__":
    main()


Review comment (suggestion):
Can you use either `check_call` (e.g., as in fix_cc_deps) or `execv` (e.g., as in run_bazelisk.py)?

Note in particular both would avoid `sys.exit`

Code after:
#!/usr/bin/env python3

"""Verify that the bazel build graph is in a valid state, for pre-commit."""

__copyright__ = """
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
"""

import subprocess

import scripts_utils


def main() -> None:
    scripts_utils.chdir_repo_root()
    bazel = scripts_utils.locate_bazel()
    subprocess.check_call([bazel, "build", "--nobuild", "//..."])


if __name__ == "__main__":
    main()
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/clang_decl.h (C/C++)

Code before:
  // This is stored along side the `decl` pointer to avoid having to lookup both
  // the pointer and the instruction ID in two separate areas of storage.
  InstId inst_id;
};

// The ID of a `ClangDecl`.
//
// These IDs are importantly distinct from the `inst_id` associated with each
// declaration. These form a dense range of IDs that is used to reference the
// AST node pointers without storing those pointers directly into SemIR and
// needing space to hold a full pointer. We can't avoid having these IDs without
// embedding pointers directly into the storage of SemIR as part of an
// instruction.
struct ClangDeclId : public IdBase<ClangDeclId> {
  static constexpr llvm::StringLiteral Label = "clang_decl_id";

  using ValueType = ClangDecl;

  // Use the AST node pointer directly when doing `Lookup` to find an ID.
  using KeyType = clang::Decl*;

  using IdBase::IdBase;
};

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_CLANG_DECL_H_


Review comment (suggestion):
I think @jonmeow raised a concern that we should change the ID as it could be used in multiple value stores, but perhaps just defining the key type is less of an issue.
https://discord.com/channels/655572317891461132/768530752592805919/1387492664458612938

Code after:
  // TODO: Ensure we can easily serialize/deserialize this. Consider
  // `clang::LazyDeclPtr`.
  clang::Decl* decl = nullptr;

  // The instruction the Clang declaration is mapped to.
  //
  // This is stored along side the `decl` pointer to avoid having to lookup both
  // the pointer and the instruction ID in two separate areas of storage.
  InstId inst_id;
};

// The ID of a `ClangDecl`.
//
// These IDs are importantly distinct from the `inst_id` associated with each
// declaration. These form a dense range of IDs that is used to reference the
// AST node pointers without storing those pointers directly into SemIR and
// needing space to hold a full pointer. We can't avoid having these IDs without
// embedding pointers directly into the storage of SemIR as part of an
// instruction.
struct ClangDeclId : public IdBase<ClangDeclId> {
  static constexpr llvm::StringLiteral Label = "clang_decl_id";

  using ValueType = ClangDecl;

  // Use the AST node pointer directly when doing `Lookup` to find an ID.
  using KeyType = clang::Decl*;

  using IdBase::IdBase;
};

}  // namespace Carbon::SemIR

#endif  // CARBON_TOOLCHAIN_SEM_IR_CLANG_DECL_H_
Repository: carbon-language/carbon-lang
File: scripts/lldbinit.py (Python)

Code before:
#!/usr/bin/env python3

"""Initialization for lldb."""

__copyright__ = """
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
"""

# This script is only meant to be used from LLDB.
import lldb  # type: ignore
import os

project_root = os.path.dirname(os.path.realpath(__file__))

ci = lldb.debugger.GetCommandInterpreter()
result = lldb.SBCommandReturnObject()


def RunCommand(cmd: str) -> None:
    """Runs a command and prints it to the console to show that it ran."""
    print("(lldb) %s" % cmd)
    ci.HandleCommand(cmd, result)


RunCommand(f"settings append target.source-map . {project_root}")
RunCommand(f"settings append target.source-map /proc/self/cwd {project_root}")


Review comment (security):
FWIW, a security scan flagged that this RE could have exponential backtracking -- clearly not a security issue for the LLDB script, and no idea if it's realistic or useful to fix, but wanted to pass it along.

Specifically the `"((:?.|\n)+)"`:

> This part of the regular expression may cause exponential backtracking on strings starting with '(std::string) "' and containing many repetitions of ':a'.

Code after:
# This script is only meant to be used from LLDB.
import lldb  # type: ignore
import os
import re
from typing import Any

project_root = os.path.dirname(os.path.realpath(__file__))

ci = lldb.debugger.GetCommandInterpreter()
result = lldb.SBCommandReturnObject()


def RunCommand(cmd: str, print_command: bool = True) -> Any:
    """Runs a command and prints it to the console to show that it ran."""
    if print_command:
        print(f"(lldb) {cmd}")
    ci.HandleCommand(cmd, result)
    return result.GetOutput()


RunCommand(f"settings append target.source-map . {project_root}")
RunCommand(f"settings append target.source-map /proc/self/cwd {project_root}")

# Matches the output of `print Dump(...)` and captures the stuff from inside the
# std::string while discarding the std::string type.
dump_re = re.compile('\\(std::string\\) "((:?.|\n)+)"', re.MULTILINE)


# A helper to ease calling the Dump() free functions.
def cmd_dump(debugger: Any, command: Any, result: Any, dict: Any) -> None:
    def print_usage() -> None:
        print(
            """
Dumps the value of an associated ID, using the C++ Dump() functions.

Usage:
  dump <CONTEXT> [<EXPR>|-- <EXPR>|<TYPE><ID>|<TYPE> <ID>]

Args:
  CONTEXT is the dump context, such a SemIR::Context reference, a SemIR::File,
          a Parse::Context, or a Lex::TokenizeBuffer.
  EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
       being treated as a TYPE and ID.
  TYPE can be `inst`, `constant`, `generic`, `impl`, `entity_name`, etc. See
       the `Label` string in `IdBase` classes to find possible TYPE names,
       though only Id types that have a matching `Make...Id()` function are
       supported.
  ID is an integer number, such as `42`.

Example usage:
  # Dumps the `inst_id` local variable, with a `context` local variable.