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

Code before:
        log_to = subprocess.DEVNULL
    generated_file_labels = subprocess.check_output(
        [bazel, "query", "--keep_going", "--output=label", kinds_query],
        stderr=log_to,
        encoding="utf-8",
    ).splitlines()
    print(f"Found {len(generated_file_labels)} generated files...")

    # Directly build these labels so that indexing can find them. Allow this to
    # fail in case there are build errors in the client, and just warn the user
    # that they may be missing generated files.
    subprocess.check_call(
        [bazel, "build", "--keep_going"] + generated_file_labels
    )


def main() -> None:
    parser = argparse.ArgumentParser(
        description="Generates compilation data used by clangd.",
        allow_abbrev=False,
    )
    parser.add_argument(
        "--alsologtostderr",
        action="store_true",
        # type=bool,
        default=False,
        help="Prints subcommand errors to stderr (default: False)",
    )

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

    _build_generated_files(bazel, args.alsologtostderr)

    print("Generating compile_commands.json (may take a few minutes)...")
    subprocess.run([bazel, "run", "@hedron_compile_commands//:refresh_all"])


if __name__ == "__main__":
    main()


Review comment (suggestion):
I would skip the commented out bit, and the default is already provided by this action from my understanding.

```suggestion
```

Code after:
        log_to = subprocess.DEVNULL
    generated_file_labels = subprocess.check_output(
        [bazel, "query", "--keep_going", "--output=label", kinds_query],
        stderr=log_to,
        encoding="utf-8",
    ).splitlines()
    print(f"Found {len(generated_file_labels)} generated files...")

    # Directly build these labels so that indexing can find them. Allow this to
    # fail in case there are build errors in the client, and just warn the user
    # that they may be missing generated files.
    subprocess.check_call(
        [bazel, "build", "--keep_going"] + generated_file_labels
    )


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)",
    )

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

    _build_generated_files(bazel, args.alsologtostderr)

    print("Generating compile_commands.json (may take a few minutes)...")
    subprocess.run([bazel, "run", "@hedron_compile_commands//:refresh_all"])


if __name__ == "__main__":
    main()
Repository: carbon-language/carbon-lang
File: scripts/create_compdb.py (Python)

Code before:
        ' kind("generated file", deps(//... except //utils/tree_sitter/...))'
        ")"
    )
    log_to = None
    if not logtostderr:
        log_to = subprocess.DEVNULL
    generated_file_labels = subprocess.check_output(
        [bazel, "query", "--keep_going", "--output=label", kinds_query],
        stderr=log_to,
        encoding="utf-8",
    ).splitlines()
    print(f"Found {len(generated_file_labels)} generated files...")

    # Directly build these labels so that indexing can find them. Allow this to
    # fail in case there are build errors in the client, and just warn the user
    # that they may be missing generated files.
    subprocess.check_call(
        [bazel, "build", "--keep_going"] + generated_file_labels
    )


def main() -> None:
    parser = argparse.ArgumentParser(
        description="Generates compilation data used by clangd.",
        allow_abbrev=False,
    )
    parser.add_argument(
        "--alsologtostderr",
        action="store_true",
        # type=bool,
        default=False,
        help="Prints subcommand errors to stderr (default: False)",
    )

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

    _build_generated_files(bazel, args.alsologtostderr)

    print("Generating compile_commands.json (may take a few minutes)...")
    subprocess.run([bazel, "run", "@hedron_compile_commands//:refresh_all"])


if __name__ == "__main__":
    main()


Review comment (suggestion):
I think we usually use the doc string for the script here:

```suggestion
    parser = argparse.ArgumentParser(description=__doc__)
```

Code after:
        ' kind("generated file", deps(//... except //utils/tree_sitter/...))'
        ")"
    )
    log_to = None
    if not logtostderr:
        log_to = subprocess.DEVNULL
    generated_file_labels = subprocess.check_output(
        [bazel, "query", "--keep_going", "--output=label", kinds_query],
        stderr=log_to,
        encoding="utf-8",
    ).splitlines()
    print(f"Found {len(generated_file_labels)} generated files...")

    # Directly build these labels so that indexing can find them. Allow this to
    # fail in case there are build errors in the client, and just warn the user
    # that they may be missing generated files.
    subprocess.check_call(
        [bazel, "build", "--keep_going"] + generated_file_labels
    )


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)",
    )

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

    _build_generated_files(bazel, args.alsologtostderr)

    print("Generating compile_commands.json (may take a few minutes)...")
    subprocess.run([bazel, "run", "@hedron_compile_commands//:refresh_all"])


if __name__ == "__main__":
    main()
Repository: carbon-language/carbon-lang
File: toolchain/language_server/handle_shutdown.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/language_server/handle.h"

namespace Carbon::LanguageServer {

auto HandleShutdown(
    Context& /*context*/,
    const clang::clangd::NoParams& /*client_capabilities*/,
    llvm::function_ref<void(llvm::Expected<std::nullptr_t>)> on_done) -> void {
  on_done(nullptr);
}

}  // namespace Carbon::LanguageServer


Review comment (suggestion):
Can you please add a test of this? e.g.., you could just add `[[@LSP-CALL:shutdown]]` to https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/language_server/testdata/basics/exit.carbon. We really value tests so I think it'd be good to make sure the call is connected, even if it doesn't much (but it should have a visible reply after running autoupdate)

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/language_server/handle.h"

namespace Carbon::LanguageServer {

auto HandleShutdown(
    Context& /*context*/,
    const clang::clangd::NoParams& /*client_capabilities*/,
    llvm::function_ref<void(llvm::Expected<std::nullptr_t>)> on_done) -> void {
  // TODO: Track that `shutdown` was called, and:
  // - Warn on duplicate calls.
  // - Make `exit` return `1` if `shutdown` wasn't called.
  //   https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit
  // - Error on other post-`shutdown` calls.
  on_done(nullptr);
}

}  // namespace Carbon::LanguageServer
Repository: carbon-language/carbon-lang
File: toolchain/language_server/handle_shutdown.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/language_server/handle.h"

namespace Carbon::LanguageServer {

auto HandleShutdown(
    Context& /*context*/,
    const clang::clangd::NoParams& /*client_capabilities*/,
    llvm::function_ref<void(llvm::Expected<std::nullptr_t>)> on_done) -> void {
  on_done(nullptr);
}

}  // namespace Carbon::LanguageServer


Review comment (bug):
Looking at the `shutdown` and `exit` spec, I think there are a few missing details here that it'd be good to have a TODO to flesh out:

```suggestion
  // TODO: Track that `shutdown` was called, and:
  // - Warn on duplicate calls.
  // - Make `exit` return `1` if `shutdown` wasn't called.
  //   https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit
  // - Error on other post-`shutdown` calls.
  on_done(nullptr);
```

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/language_server/handle.h"

namespace Carbon::LanguageServer {

auto HandleShutdown(
    Context& /*context*/,
    const clang::clangd::NoParams& /*client_capabilities*/,
    llvm::function_ref<void(llvm::Expected<std::nullptr_t>)> on_done) -> void {
  // TODO: Track that `shutdown` was called, and:
  // - Warn on duplicate calls.
  // - Make `exit` return `1` if `shutdown` wasn't called.
  //   https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit
  // - Error on other post-`shutdown` calls.
  on_done(nullptr);
}

}  // namespace Carbon::LanguageServer
Repository: carbon-language/carbon-lang
File: testing/base/file_helpers.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/base/file_helpers.h"

#include <fstream>
#include <sstream>

namespace Carbon::Testing {

auto ReadFile(std::filesystem::path path) -> ErrorOr<std::string> {
  std::ifstream file_stream(path);
  if (file_stream.fail()) {
    return Error(llvm::formatv("Error opening file: {0}", path));
  }
  std::stringstream buffer;
  buffer << file_stream.rdbuf();
  if (file_stream.fail()) {
    return Error(llvm::formatv("Error reading file: {0}", path));
  }
  file_stream.close();
  return buffer.str();
}

}  // namespace Carbon::Testing


Review comment (suggestion):
You don't need to do this; the destructor closes the file.
```suggestion
```

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/base/file_helpers.h"

#include <fstream>
#include <sstream>

namespace Carbon::Testing {

auto ReadFile(std::filesystem::path path) -> ErrorOr<std::string> {
  std::ifstream file_stream(path);
  if (file_stream.fail()) {
    return Error(llvm::formatv("Error opening file: {0}", path));
  }
  std::stringstream buffer;
  buffer << file_stream.rdbuf();
  if (file_stream.fail()) {
    return Error(llvm::formatv("Error reading file: {0}", path));
  }
  return buffer.str();
}

}  // namespace Carbon::Testing
Repository: carbon-language/carbon-lang
File: testing/base/file_helpers.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_TESTING_BASE_FILE_HELPERS_H_
#define CARBON_TESTING_BASE_FILE_HELPERS_H_

#include <filesystem>
#include <string>

#include "common/error.h"

namespace Carbon::Testing {

// Reads a file to string.
auto ReadFile(std::filesystem::path path) -> ErrorOr<std::string>;

}  // namespace Carbon::Testing

#endif  // CARBON_TESTING_BASE_FILE_HELPERS_H_


Review comment (suggestion):
Just to note, if you touch this again, you might call it `WriteFile` to match `ReadFile` (it's already in the `Testing` namespace)

Code after:

#include <filesystem>
#include <string>

#include "common/error.h"

namespace Carbon::Testing {

// Reads a file to string.
auto ReadFile(std::filesystem::path path) -> ErrorOr<std::string>;

// Writes a test file to disk and returns an error or the full path to the file.
//
// Note that this expects to be run from within a GoogleTest test, and relies on
// global state of GoogleTest.
//
// This locates a suitable temporary directory for the test, creates a file with
// the requested name in that directory, and writes the provided content to that
// file. The full path to the written file is returned.
//
// Where possible, this will use a Bazel-provided test temporary directory.
// However, if unavailable, it falls back to a system temporary directory. This
// helps tests be runnable outside of Bazel, for example under a debugger. It
// also works to create test file names that are unlikely to conflict with other
// tests when run.
auto WriteTestFile(llvm::StringRef name, llvm::StringRef contents)
    -> ErrorOr<std::filesystem::path>;

}  // namespace Carbon::Testing

#endif  // CARBON_TESTING_BASE_FILE_HELPERS_H_
Repository: carbon-language/carbon-lang
File: testing/file_test/file_test_base.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

// Implementation-wise, what this does is:
//
// - Uses the registered `FileTestFactory` to construct `FileTestBase`
//   instances.
// - Constructs a `FileTestCase` that wraps each `FileTestBase` instance to
//   register with googletest, and to provide the actual `TestBody`.
// - Using `FileTestEventListener`, runs tests in parallel prior to normal
//   googletest execution.
//   - This is required to support `--gtest_filter` and access `should_run`.
//   - Runs each `FileTestBase` instance to cache the `TestFile` on
//     `FileTestInfo`.
//   - Determines whether autoupdate would make changes, autoupdating if
//     requested.
// - When googletest would normally execute the test, `FileTestCase::TestBody`
//   instead uses the cached state on `FileTestInfo`.
//   - This only occurs when neither autoupdating nor dumping output.

#include "testing/file_test/file_test_base.h"

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

#include "absl/flags/flag.h"
#include "absl/flags/parse.h"

Review comment (suggestion):
```suggestion
// Implementation-wise, this:
```

(Grammar nit; the alternative fix would be to give the bulleted sentences subjects)

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

// Implementation-wise, this:
//
// - Uses the registered `FileTestFactory` to construct `FileTestBase`
//   instances.
// - Constructs a `FileTestCase` that wraps each `FileTestBase` instance to
//   register with googletest, and to provide the actual `TestBody`.
// - Using `FileTestEventListener`, runs tests in parallel prior to normal
//   googletest execution.
//   - This is required to support `--gtest_filter` and access `should_run`.
//   - Runs each `FileTestBase` instance to cache the `TestFile` on
//     `FileTestInfo`.
//   - Determines whether autoupdate would make changes, autoupdating if
//     requested.
// - When googletest would normally execute the test, `FileTestCase::TestBody`
//   instead uses the cached state on `FileTestInfo`.
//   - This only occurs when neither autoupdating nor dumping output.

#include "testing/file_test/file_test_base.h"

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

#include "absl/flags/flag.h"
#include "absl/flags/parse.h"
Repository: carbon-language/carbon-lang
File: toolchain/lower/mangler.cpp (C++)

Code before:
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_clang_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    cpp_clang_mangle_context_.reset(
        decl->getASTContext().createMangleContext());
  }

  std::string cpp_mangled_name;
  llvm::raw_string_ostream os(cpp_mangled_name);
  cpp_clang_mangle_context_->mangleName(decl, os);

  return cpp_mangled_name;
}

}  // namespace Carbon::Lower


Review comment (refactor):
```suggestion
  RawStringOstream cpp_mangled_name;
  cpp_clang_mangle_context_->mangleName(decl, cpp_mangled_name);

  return cpp_mangled_name.TakeStr();
```

Utility class

Code after:
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    // TODO: Consider initializing this in the constructor. This is related to:
    // https://github.com/carbon-language/carbon-lang/issues/4666. See
    // https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815
    cpp_mangle_context_.reset(decl->getASTContext().createMangleContext());
  }

  RawStringOstream cpp_mangled_name;
  cpp_mangle_context_->mangleName(decl, cpp_mangled_name);

  return cpp_mangled_name.TakeStr();
}

}  // namespace Carbon::Lower
Repository: carbon-language/carbon-lang
File: toolchain/lower/mangler.cpp (C++)

Code before:

  os << names().GetAsStringIfIdentifier(function.name_id);

  MangleInverseQualifiedNameScope(os, function.parent_scope_id);

  // TODO: Add proper support for mangling generic entities. For now we use a
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_clang_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    cpp_clang_mangle_context_.reset(
        decl->getASTContext().createMangleContext());
  }

  std::string cpp_mangled_name;
  llvm::raw_string_ostream os(cpp_mangled_name);
  cpp_clang_mangle_context_->mangleName(decl, os);

  return cpp_mangled_name;
}

}  // namespace Carbon::Lower


Review comment (suggestion):
Had you considered initializing this on construction from the SemIR's AST? i.e., instead of lazily constructing from the first decl's AST context pointer, instead construct from `sem_ir().cpp_ast()->getASTContext().createMangleContext());`.

Code after:

  os << names().GetAsStringIfIdentifier(function.name_id);

  MangleInverseQualifiedNameScope(os, function.parent_scope_id);

  // TODO: Add proper support for mangling generic entities. For now we use a
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    // TODO: Consider initializing this in the constructor. This is related to:
    // https://github.com/carbon-language/carbon-lang/issues/4666. See
    // https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815
    cpp_mangle_context_.reset(decl->getASTContext().createMangleContext());
  }

  RawStringOstream cpp_mangled_name;
  cpp_mangle_context_->mangleName(decl, cpp_mangled_name);

  return cpp_mangled_name.TakeStr();
}

}  // namespace Carbon::Lower
Repository: carbon-language/carbon-lang
File: toolchain/lower/mangler.cpp (C++)

Code before:
  RawStringOstream os;
  os << "_C";

  os << names().GetAsStringIfIdentifier(function.name_id);

  MangleInverseQualifiedNameScope(os, function.parent_scope_id);

  // TODO: Add proper support for mangling generic entities. For now we use a
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    // TODO: #4666 Consider initializing this in the constructor.
    cpp_mangle_context_.reset(decl->getASTContext().createMangleContext());
  }

  RawStringOstream cpp_mangled_name;
  cpp_mangle_context_->mangleName(decl, cpp_mangled_name);

  return cpp_mangled_name.TakeStr();
}

}  // namespace Carbon::Lower


Review comment (suggestion):
```suggestion
    // TODO: Consider initializing this in the constructor.
```

It seems like #4666 doesn't talk about this, so maybe it's not useful to mention here?

Note, if you want the connection for some reason, please write it as a sentence with a link (don't assume people know what "4666" means, and make it easy for even the people who know to look in GitHub to get there). For example:

```
    // TODO: Consider initializing this in the constructor. This is related to:
    // https://github.com/carbon-language/carbon-lang/issues/4666
```

Code after:
  RawStringOstream os;
  os << "_C";

  os << names().GetAsStringIfIdentifier(function.name_id);

  MangleInverseQualifiedNameScope(os, function.parent_scope_id);

  // TODO: Add proper support for mangling generic entities. For now we use a
  // fingerprint of the specific arguments, which should be stable across files,
  // but isn't necessarily stable across toolchain changes.
  if (specific_id.has_value()) {
    os << ".";
    llvm::write_hex(
        os,
        fingerprinter_.GetOrCompute(
            &sem_ir(), sem_ir().specifics().Get(specific_id).args_id),
        llvm::HexPrintStyle::Lower, 16);
  }

  return os.TakeStr();
}

auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string {
  if (!cpp_mangle_context_) {
    // We assume all declarations are from the same AST Context.
    // TODO: Consider initializing this in the constructor. This is related to:
    // https://github.com/carbon-language/carbon-lang/issues/4666. See
    // https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815
    cpp_mangle_context_.reset(decl->getASTContext().createMangleContext());
  }

  RawStringOstream cpp_mangled_name;
  cpp_mangle_context_->mangleName(decl, cpp_mangled_name);

  return cpp_mangled_name.TakeStr();
}

}  // namespace Carbon::Lower