ronantakizawa/codereview-bench huggingface dataset:
Repository: apple/swift-nio
File: Tests/NIOFileSystemIntegrationTests/XCTestExtensions.swift (Swift)

Code before:
            )
        }
    }
}

@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
func XCTAssertThrowsFileSystemErrorAsync<R>(
    file: StaticString = #file,
    line: UInt = #line,
    _ expression: () async throws -> R,
    onError: (FileSystemError) -> Void = { _ in }
) async {
    await XCTAssertThrowsErrorAsync(file: file, line: line, expression: expression) { error in
        if let fsError = error as? FileSystemError {
            onError(fsError)
        } else {
            XCTFail(
                "Expected 'FileSystemError' but found '\(type(of: error))'",
                file: file,
                line: line
            )
        }
    }
}
#endif


Review comment (suggestion):
we shouldn't squat on XCTest's `XCT*` name space, you can make it `NIOAssertNoThrowAsync` or smth

Code after:
            )
        }
    }
}

@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
func XCTAssertThrowsFileSystemErrorAsync<R>(
    file: StaticString = #file,
    line: UInt = #line,
    _ expression: () async throws -> R,
    onError: (FileSystemError) -> Void = { _ in }
) async {
    await XCTAssertThrowsErrorAsync(file: file, line: line, expression: expression) { error in
        if let fsError = error as? FileSystemError {
            onError(fsError)
        } else {
            XCTFail(
                "Expected 'FileSystemError' but found '\(type(of: error))'",
                file: file,
                line: line
            )
        }
    }
}

func XCTAssertNoThrowAsync<T>(
    _ expression: @autoclosure () async throws -> T,
    file: StaticString = #file,
    line: UInt = #line
) async {
    do {
        _ = try await expression()
    } catch {
        XCTFail("Expression did throw: \(error)", file: file, line: line)
    }
}
#endif
Repository: apple/swift-nio
File: Sources/NIOWebSocket/WebSocketFrame.swift (Swift)

Code before:
        """
    }
}

extension WebSocketFrame: CustomDebugStringConvertible {
    public var debugDescription: String {
        "(\(self.description))"
    }
}

extension WebSocketFrame {
    /// WebSocketFrame reserved bits option set
    public struct ReservedBits: OptionSet, Sendable {
        public var rawValue: UInt8

        public init(rawValue: UInt8) {
            self.rawValue = rawValue
        }

        public static var rsv1: Self { .init(rawValue: 0x40) }
        public static var rsv2: Self { .init(rawValue: 0x20) }
        public static var rsv3: Self { .init(rawValue: 0x10) }
        public static var all: Self { .init(rawValue: 0x70) }
    }

    /// The value of all the reserved bits. Must be `empty` unless using an extension that defines their use.
    public var reservedBits: ReservedBits {
        get {
            .init(rawValue: self.firstByte & 0x70)
        }
        set {
            self.firstByte = (self.firstByte & 0x8F) + newValue.rawValue
        }
    }
}


Review comment (performance):
Let's make this `@inlinable` and all the `static var`s as well. That unlocks the meaningful performance improvement.

Code after:
        rsv1: \(self.rsv1), \
        rsv2: \(self.rsv2), \
        rsv3: \(self.rsv3), \
        opcode: \(self.opcode), \
        length: \(self.length), \
        data: \(String(describing: self.data)), \
        extensionData: \(String(describing: self.extensionData)), \
        unmaskedData: \(String(describing: self.unmaskedData)), \
        unmaskedDataExtension: \(String(describing: self.unmaskedExtensionData))
        """
    }
}

extension WebSocketFrame: CustomDebugStringConvertible {
    public var debugDescription: String {
        "(\(self.description))"
    }
}

extension WebSocketFrame {
    /// WebSocketFrame reserved bits option set
    public struct ReservedBits: OptionSet, Sendable {
        public var rawValue: UInt8

        @inlinable
        public init(rawValue: UInt8) {
            self.rawValue = rawValue
        }

        @inlinable
        public static var rsv1: Self { .init(rawValue: 0x40) }
        @inlinable
        public static var rsv2: Self { .init(rawValue: 0x20) }
        @inlinable
        public static var rsv3: Self { .init(rawValue: 0x10) }
        @inlinable
        public static var all: Self { .init(rawValue: 0x70) }
    }

    /// The value of all the reserved bits. Must be `empty` unless using an extension that defines their use.
    @inlinable
    public var reservedBits: ReservedBits {
        get {
            .init(rawValue: self.firstByte & 0x70)
        }
        set {
            self.firstByte = (self.firstByte & 0x8F) + newValue.rawValue
        }
    }
}
Repository: apple/swift-nio
File: scripts/check-cxx-interop-compatibility.sh (Shell)

Code before:

set -euo pipefail

log() { printf -- "** %s\n" "$*" >&2; }
error() { printf -- "** ERROR: %s\n" "$*" >&2; }
fatal() { error "$@"; exit 1; }

log "Checking for Cxx interoperability compatibility..."

source_dir=$(pwd)
working_dir=$(mktemp -d)
project_name=$(basename "$working_dir")
source_file=Sources/$project_name/$(echo "$project_name" | tr . _).swift
library_products=$( swift package dump-package | jq -r '.products[] | select(.type.library != null) | .name')
package_name=$(swift package dump-package | jq -r '.name')

cd "$working_dir"
swift package init
echo "package.dependencies.append(.package(path: \"$source_dir\"))" >> Package.swift

for product in $library_products; do
  echo "package.targets.first!.dependencies.append(.product(name: \"$product\", package: \"$package_name\"))" >> Package.swift
  echo "import $product" >> "$source_file"
done

# A platform independent (but brittle) way to add the swift settings block
sed '/name: "'"${project_name}"'")/a\
        swiftSettings: [\
            .interoperabilityMode(.Cxx)\
        ]),\
' Package.swift >| Package.swift.new && mv -f Package.swift.new Package.swift
sed  's/name: "'"${project_name}"'")/name: "'"${project_name}"'"/' Package.swift >| Package.swift.new && mv -f Package.swift.new Package.swift

if [[ ! $(grep interoperabilityMode Package.swift) ]]; then
  cat Package.swift
  fatal "Package.swift modification failed"
fi

swift build

log "✅ Passed the Cxx interoperability tests."


Review comment (suggestion):
Can't we use the fact that `Package` has reference semantics and just add settings after the fact?

i.e something like:

```swift
let package = Package(...)

// Generate this bit:
for target in package.targets {
  target.swiftSettings = [....]
}

Code after:

set -euo pipefail

log() { printf -- "** %s\n" "$*" >&2; }
error() { printf -- "** ERROR: %s\n" "$*" >&2; }
fatal() { error "$@"; exit 1; }

log "Checking for Cxx interoperability compatibility..."

source_dir=$(pwd)
working_dir=$(mktemp -d)
project_name=$(basename "$working_dir")
source_file=Sources/$project_name/$(echo "$project_name" | tr . _).swift
library_products=$( swift package dump-package | jq -r '.products[] | select(.type.library != null) | .name')
package_name=$(swift package dump-package | jq -r '.name')

cd "$working_dir"
swift package init

{
  echo "let swiftSettings: [SwiftSetting] = [.interoperabilityMode(.Cxx)]"
  echo "for target in package.targets { target.swiftSettings = swiftSettings }"
} >> Package.swift

echo "package.dependencies.append(.package(path: \"$source_dir\"))" >> Package.swift

for product in $library_products; do
  echo "package.targets.first!.dependencies.append(.product(name: \"$product\", package: \"$package_name\"))" >> Package.swift
  echo "import $product" >> "$source_file"
done

swift build

log "✅ Passed the Cxx interoperability tests."
Repository: apple/swift-nio
File: Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift (Swift)

Code before:
    return copyfile(from, to, state, flags)
}
#endif

/// getcwd(3): Get working directory pathname
internal func libc_getcwd(
    _ buffer: UnsafeMutablePointer<CInterop.PlatformChar>,
    _ size: Int
) -> UnsafeMutablePointer<CInterop.PlatformChar>? {
    getcwd(buffer, size)
}

/// confstr(3)
#if !os(Android)
internal func libc_confstr(
    _ name: CInt,
    _ buffer: UnsafeMutablePointer<CInterop.PlatformChar>,
    _ size: Int
) -> Int {
    confstr(name, buffer, size)
}
#endif

/// fts(3)
#if os(Android)
internal func libc_fts_open(
    _ path: [UnsafeMutablePointer<CInterop.PlatformChar>],
    _ options: CInt
) -> UnsafeMutablePointer<CInterop.FTS> {
    fts_open(path, options, nil)!
}
#else
internal func libc_fts_open(
    _ path: [UnsafeMutablePointer<CInterop.PlatformChar>?],
    _ options: CInt
) -> UnsafeMutablePointer<CInterop.FTS> {
    fts_open(path, options, nil)
}
#endif

/// fts(3)
internal func libc_fts_read(
    _ fts: UnsafeMutablePointer<CInterop.FTS>
) -> UnsafeMutablePointer<CInterop.FTSEnt>? {
    fts_read(fts)
}

/// fts(3)
internal func libc_fts_close(
    _ fts: UnsafeMutablePointer<CInterop.FTS>
) -> CInt {

Review comment (bug):
This was [incorrectly marked as `_Nonnull` in recent Android NDKs, which will be fixed in the upcoming NDK 28](https://android.googlesource.com/platform/bionic/+/da81ec4d1cbd0279014feb60535bf38defcd9346%5E%21/#F0), but in the meantime, Alex added an API note to fix it for 6.1, [which is about to be backported to 6.0 also](https://github.com/swiftlang/swift/pull/77626/files#diff-80ad50e239d0b1f4236139c396a2f6e9e53dd96c3726de022e4efa7b4b2e1addR4).

Once it is, the alternate Android definition above is unneeded for Swift 6, so I'm simply doing the force-unwrap for Android here instead.

Code after:
    return copyfile(from, to, state, flags)
}
#endif

/// getcwd(3): Get working directory pathname
internal func libc_getcwd(
    _ buffer: UnsafeMutablePointer<CInterop.PlatformChar>,
    _ size: Int
) -> UnsafeMutablePointer<CInterop.PlatformChar>? {
    getcwd(buffer, size)
}

/// confstr(3)
#if !os(Android)
internal func libc_confstr(
    _ name: CInt,
    _ buffer: UnsafeMutablePointer<CInterop.PlatformChar>,
    _ size: Int
) -> Int {
    confstr(name, buffer, size)
}
#endif

/// fts(3)
internal func libc_fts_open(
    _ path: [UnsafeMutablePointer<CInterop.PlatformChar>?],
    _ options: CInt
) -> UnsafeMutablePointer<CInterop.FTS> {
    fts_open(path, options, nil)!
}

/// fts(3)
internal func libc_fts_read(
    _ fts: UnsafeMutablePointer<CInterop.FTS>
) -> UnsafeMutablePointer<CInterop.FTSEnt>? {
    fts_read(fts)
}

/// fts(3)
internal func libc_fts_close(
    _ fts: UnsafeMutablePointer<CInterop.FTS>
) -> CInt {
    fts_close(fts)
}
Repository: apple/swift-nio
File: Sources/NIOFileSystem/String+FileSystem.swift (Swift)

Code before:
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2024 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

import NIOCore

@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
extension String {
    /// Reads the contents of the file at the path into a String.
    ///
    /// - Parameters:
    ///   - path: The path of the file to read.
    ///   - maximumSizeAllowed: The maximum size of file which can be read, in bytes, as a ``ByteCount``. If the file is larger than this, an error is thrown.
    ///   - fileSystem: The ``FileSystemProtocol`` instance to use to read the file.
    ///
    /// - Throws: If the file is larger than `maximumSizeAllowed`, an ``FileSystemError.resourceExhausted`` error will be thrown.
    public init(
        contentsOf path: FilePath,
        maximumSizeAllowed: ByteCount,
        fileSystem: some FileSystemProtocol

Review comment (security):
```suggestion
// Copyright (c) 2025 Apple Inc. and the SwiftNIO project authors
```

Code after:
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2025 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

import NIOCore

@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
extension String {
    /// Reads the contents of the file at the path into a String.
    ///
    /// - Parameters:
    ///   - path: The path of the file to read.
    ///   - maximumSizeAllowed: The maximum size of file which can be read, in bytes, as a ``ByteCount``. If the file is larger than this, an error is thrown.
    ///   - fileSystem: The ``FileSystemProtocol`` instance to use to read the file.
    ///
    /// - Throws: If the file is larger than `maximumSizeAllowed`, an ``FileSystemError/Code-swift.struct/resourceExhausted`` error will be thrown.
    public init(
        contentsOf path: FilePath,
        maximumSizeAllowed: ByteCount,
        fileSystem: some FileSystemProtocol
Repository: apple/swift-nio
File: Sources/NIOTestUtils/ManualTaskExecutor.swift (Swift)

Code before:
    /// Called by the concurrency runtime.
    ///
    /// - Parameter job: The job to enqueue.
    public func enqueue(_ job: UnownedJob) {
        self.storage.withLock { storage in
            if storage.isShutdown {
                fatalError("A job is enqueued after manual executor shutdown")
            }
            storage.jobs.append(job)
        }
    }

    /// Shutdown.
    ///
    /// Since the manual task executor is not running anything in the background, this is purely to catch
    /// any issues due to incorrect usage of the executor. The shutdown verifies that the queue is empty
    /// and makes sure that no new jobs can be enqueued.
    @usableFromInline
    func shutdown() {
        self.storage.withLock { storage in
            if !storage.jobs.isEmpty {
                fatalError("Shutdown of manual executor with jobs in queue")
            }
            storage.isShutdown = true
        }
    }
}

#endif  // compiler(>=6)


Review comment (refactor):
I don't know if we really need this complicated shutdown handling. Since task executors can only be set on a scope we shouldn't have to do any manual clean up or reject jobs.

Code after:
    ///
    /// Called by the concurrency runtime.
    ///
    /// - Parameter job: The job to enqueue.
    @usableFromInline
    package func enqueue(_ job: UnownedJob) {
        self.storage.withLock { storage in
            if storage.isShutdown {
                fatalError("A job is enqueued after manual executor shutdown")
            }
            storage.jobs.append(job)
        }
    }

    /// Shutdown.
    ///
    /// Since the manual task executor is not running anything in the background, this is purely to catch
    /// any issues due to incorrect usage of the executor. The shutdown verifies that the queue is empty
    /// and makes sure that no new jobs can be enqueued.
    @usableFromInline
    func shutdown() {
        self.storage.withLock { storage in
            if !storage.jobs.isEmpty {
                fatalError("Shutdown of manual executor with jobs in queue")
            }
            storage.isShutdown = true
        }
    }
}

#endif  // compiler(>=6)
Repository: apple/swift-nio
File: scripts/cmake-build.sh (Shell)

Code before:
set -uo pipefail

log() { printf -- "** %s\n" "$*" >&2; }
error() { printf -- "** ERROR: %s\n" "$*" >&2; }
fatal() { error "$@"; exit 1; }

version="${CMAKE_VERSION:="3.31.6"}"
target_dir="${TARGET_DIRECTORY:=""}"

if [ -z "$version" ]; then
  fatal "CMake version must be specified."
fi
if [ -z "$target_dir" ]; then
  fatal "Target directory must be specified."
fi

log "Installing tools for this script"
apt-get update -y -q && apt-get install -y -q curl rsync

log "Obtaining CMake (${CMAKE_VERSION})"
curl -sL "https://github.com/Kitware/CMake/releases/download/v${CMAKE_VERSION}/cmake-${CMAKE_VERSION}-linux-x86_64.tar.gz" -o /tmp/cmake.tar.gz

log "Installing CMake"
mkdir -p /tmp/cmake
tar xfvz /tmp/cmake.tar.gz --strip-components 1 -C /tmp/cmake
rsync -q -a /tmp/cmake/* /usr/

log "Installing Ninja"
apt-get install -y -q ninja-build

log "Building makefiles for target"
build_directory="${TARGET_DIRECTORY}/build"
mkdir -p "${build_directory}"
cd "${build_directory}" || fatal "Could not 'cd' to ${build_directory}"
cmake build -G Ninja -S ..

log "Building target"
ninja


Review comment (refactor):
This seems a bit of an unnecessary indirection. If we're extracting to a temporary and immediately shovelling everything into `/usr/`, can't we just extract there?

Code after:
set -uo pipefail

log() { printf -- "** %s\n" "$*" >&2; }
error() { printf -- "** ERROR: %s\n" "$*" >&2; }
fatal() { error "$@"; exit 1; }

target_dir="${TARGET_DIRECTORY:=""}"

if [ -z "$target_dir" ]; then
  fatal "Target directory must be specified."
fi

CURL_BIN="${CURL_BIN:-$(which curl)}" || fatal "CURL_BIN unset and no curl on PATH"
TAR_BIN="${TAR_BIN:-$(which tar)}" || fatal "TAR_BIN unset and no tar on PATH"
CMAKE_BIN="${CMAKE_BIN:-$(which cmake)}" || fatal "CMAKE_BIN unset and no cmake on PATH"
NINJA_BIN="${NINJA_BIN:-$(which ninja)}" || fatal "NINJA_BIN unset and no ninja on PATH"
ASSEMBLY_COMPILER_BIN="${ASSEMBLY_COMPILER_BIN:-$(which clang)}" || fatal "ASSEMBLY_COMPILER_BIN unset and no clang on PATH"

log "Building Ninja build files for target"
build_dir="${target_dir}/build"
mkdir -p "$build_dir"
cd "${build_dir}" || fatal "Could not 'cd' to ${build_dir}"
ASM="$ASSEMBLY_COMPILER_BIN" "$CMAKE_BIN" build -G Ninja -S ..

log "Building target"
"$NINJA_BIN"
Repository: apple/swift-nio
File: dev/thresholds-from-benchmark-output.sh (Shell)

Code before:

url="${URL:=""}"

if [ -z "$url" ]; then
  fatal "Pull request or workflow run URL must be specified."
fi

# Check for required tools
GH_BIN="${GH_BIN:-$(which gh)}" || fatal "GH_BIN unset and no gh on PATH"
JQ_BIN="${JQ_BIN:-$(which jq)}" || fatal "JQ_BIN unset and no jq on PATH"
YQ_BIN="${YQ_BIN:-$(which yq)}" || fatal "YQ_BIN unset and no yq on PATH"

fetch_checks_for_pr() {
    pr_url=$1

    "$GH_BIN" pr checks "$pr_url" | grep Benchmarks | grep -v Construct
}

parse_url() {
    workflow_url=$1
    # https://github.com/apple/swift-nio/actions/runs/15269806473
    # https://github.com/apple/swift-nio/pull/3257
    if echo "$url" | grep -q "pull"; then
        type="PR"
    elif echo "$url" | grep -q "actions/runs"; then
        type="run"
    else
        fatal "Cannot parse URL: $url"
    fi
    echo "$url" | awk -v type="$type" -F '/' '{print $4, $5, type}'
}

parse_check() {
    type=$1
    check_line=$2

    case "$type" in
    "PR")
        parse_check_for_pr "$check_line"
        ;;

    "run")
        parse_check_for_workflow "$check_line"
        ;;

    *)
        fatal "Unknown type '$type'"
        # Add error handling commands here
        ;;
    esac
}

Review comment (suggestion):
FWIW you don't need grep here:

```suggestion
    if [[ "$url" =~ pull ]]; then
        type="PR"
    elif [[ "$url" =~ actions/runs ]]; then
        type="run"
```

Code after:

url="${URL:=""}"

if [ -z "$url" ]; then
  fatal "Pull request or workflow run URL must be specified."
fi

# Check for required tools
GH_BIN="${GH_BIN:-$(which gh)}" || fatal "GH_BIN unset and no gh on PATH"
JQ_BIN="${JQ_BIN:-$(which jq)}" || fatal "JQ_BIN unset and no jq on PATH"
YQ_BIN="${YQ_BIN:-$(which yq)}" || fatal "YQ_BIN unset and no yq on PATH"

fetch_checks_for_pr() {
    pr_url=$1

    "$GH_BIN" pr checks "$pr_url" | grep Benchmarks | grep -v Construct
}

parse_url() {
    workflow_url=$1
    # https://github.com/apple/swift-nio/actions/runs/15269806473
    # https://github.com/apple/swift-nio/pull/3257
    if [[ "$url" =~ pull ]]; then
        type="PR"
    elif [[ "$url" =~ actions/runs ]]; then
        type="run"
    else
        fatal "Cannot parse URL: $url"
    fi
    echo "$url" | awk -v type="$type" -F '/' '{print $4, $5, type}'
}

parse_check() {
    type=$1
    check_line=$2

    case "$type" in
    "PR")
        parse_check_for_pr "$check_line"
        ;;

    "run")
        parse_check_for_workflow "$check_line"
        ;;

    *)
        fatal "Unknown type '$type'"
        # Add error handling commands here
        ;;
    esac
}
Repository: apple/swift-nio
File: Sources/NIOCore/NIOScheduledCallback.swift (Swift)

Code before:
        case .default(let task):
            task.cancel()
        case .custom:
            preconditionFailure("EventLoop missing custom implementation of cancelScheduledCallback(_:)")
        }
    }
}

@usableFromInline
struct LoopBoundScheduledCallbackHandlerWrapper<Handler: NIOScheduledCallbackHandler>: NIOScheduledCallbackHandler, Sendable {
    private let box: NIOLoopBound<Handler>

    @usableFromInline
    init(wrapping handler: Handler, eventLoop: some EventLoop) {
        self.box = .init(handler, eventLoop: eventLoop)
    }

    @usableFromInline
    func handleScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.handleScheduledCallback(eventLoop: eventLoop)
    }

    @usableFromInline
    func didCancelScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.didCancelScheduledCallback(eventLoop: eventLoop)
    }
}


Review comment (suggestion):
This type is generic over a user provided `Handler`, to ensure perf, this should not just be `@useableFromInline` it should be `@inlinable` instead.
```suggestion
    @inlinable
    init(wrapping handler: Handler, eventLoop: some EventLoop) {
        self.box = .init(handler, eventLoop: eventLoop)
    }

    @inlinable
    func handleScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.handleScheduledCallback(eventLoop: eventLoop)
    }

    @inlinable
    func didCancelScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.didCancelScheduledCallback(eventLoop: eventLoop)
    }
```

Code after:
        case .custom:
            preconditionFailure("EventLoop missing custom implementation of cancelScheduledCallback(_:)")
        }
    }
}

@usableFromInline
struct LoopBoundScheduledCallbackHandlerWrapper<Handler: NIOScheduledCallbackHandler>:
    NIOScheduledCallbackHandler, Sendable
{
    private let box: NIOLoopBound<Handler>

    @usableFromInline
    init(wrapping handler: Handler, eventLoop: some EventLoop) {
        self.box = .init(handler, eventLoop: eventLoop)
    }

    @usableFromInline
    func handleScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.handleScheduledCallback(eventLoop: eventLoop)
    }

    @usableFromInline
    func didCancelScheduledCallback(eventLoop: some EventLoop) {
        self.box.value.didCancelScheduledCallback(eventLoop: eventLoop)
    }
}