ronantakizawa/codereview-bench huggingface dataset:
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/src/internal/StackTraceRecovery.common.kt (Kotlin)

Code before:
internal expect fun <E: Throwable> recoverStackTrace(exception: E, continuation: Continuation<*>): E

/**
 * initCause on JVM, nop on other platforms
 */
@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
internal expect fun Throwable.initCause(cause: Throwable)

/**
 * Tries to recover stacktrace for given [exception]. Used in non-suspendable points of awaiting.
 * Stacktrace recovery tries to instantiate exception of given type with original exception as a cause.
 * Wrapping exception will have proper stacktrace as it's instantiated in the right context.
 *
 * Works only on JVM with enabled debug-mode.
 */
internal expect fun <E: Throwable> recoverStackTrace(exception: E): E

// Name conflict with recoverStackTrace
@Suppress("NOTHING_TO_INLINE")
internal expect suspend inline fun recoverAndThrow(exception: Throwable): Nothing

/**
 * The opposite of [recoverStackTrace].
 * It is guaranteed that `unwrap(recoverStackTrace(e)) === e`
 */
@PublishedApi // published for the multiplatform implementation of kotlinx-coroutines-test
internal expect fun <E: Throwable> unwrap(exception: E): E

internal expect class StackTraceElement

internal expect interface CoroutineStackFrame {
    public val callerFrame: CoroutineStackFrame?
    public fun getStackTraceElement(): StackTraceElement?
}


Review comment (suggestion):
```suggestion
@PublishedApi // Used from kotlinx-coroutines-test and reactor modules via suppress, not part of ABI
```

Code after:
internal expect fun <E: Throwable> recoverStackTrace(exception: E, continuation: Continuation<*>): E

/**
 * initCause on JVM, nop on other platforms
 */
@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
internal expect fun Throwable.initCause(cause: Throwable)

/**
 * Tries to recover stacktrace for given [exception]. Used in non-suspendable points of awaiting.
 * Stacktrace recovery tries to instantiate exception of given type with original exception as a cause.
 * Wrapping exception will have proper stacktrace as it's instantiated in the right context.
 *
 * Works only on JVM with enabled debug-mode.
 */
internal expect fun <E: Throwable> recoverStackTrace(exception: E): E

// Name conflict with recoverStackTrace
@Suppress("NOTHING_TO_INLINE")
internal expect suspend inline fun recoverAndThrow(exception: Throwable): Nothing

/**
 * The opposite of [recoverStackTrace].
 * It is guaranteed that `unwrap(recoverStackTrace(e)) === e`
 */
@PublishedApi // Used from kotlinx-coroutines-test and reactor modules via suppress, not part of ABI
internal expect fun <E: Throwable> unwrap(exception: E): E

internal expect class StackTraceElement

internal expect interface CoroutineStackFrame {
    public val callerFrame: CoroutineStackFrame?
    public fun getStackTraceElement(): StackTraceElement?
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/jvm/test/channels/BroadcastChannelMultiReceiveStressTest.kt (Kotlin)

Code before:
    }

    private suspend fun doReceiveSelect(channel: ReceiveChannel<Long>, receiverIndex: Int) {
        while (true) {
            try {
                val event = select<Long> { channel.onReceive { it } }
                val stop = doReceived(receiverIndex, event)
                if (stop) break
            } catch (ex: ClosedReceiveChannelException) {
                break
            }
        }
    }

    private suspend fun doReceiveCatchingSelect(channel: ReceiveChannel<Long>, receiverIndex: Int) {
        while (true) {
            val event = select<Long?> { channel.onReceiveCatching { it.getOrNull() } } ?: break
            val stop = doReceived(receiverIndex, event)
            if (stop) break
        }
    }

    @Suppress("UNUSED_PARAMETER")
    private fun println(debugMessage: String) {
        // Uncomment for local debugging
        // kotlin.io.println(debugMessage)
    }
}


Review comment (bug):
Uncommenting this leads to a crash. The correct version is `println(debugMessage as Any?)`.

Code after:
        while (true) {
            try {
                val event = select<Long> { channel.onReceive { it } }
                val stop = doReceived(receiverIndex, event)
                if (stop) break
            } catch (ex: ClosedReceiveChannelException) {
                break
            }
        }
    }

    private suspend fun doReceiveCatchingSelect(channel: ReceiveChannel<Long>, receiverIndex: Int) {
        while (true) {
            val event = select<Long?> { channel.onReceiveCatching { it.getOrNull() } } ?: break
            val stop = doReceived(receiverIndex, event)
            if (stop) break
        }
    }

    @Suppress("UNUSED_PARAMETER")
    private fun println(debugMessage: String) {
        // Uncomment for local debugging
        //println(debugMessage as Any?)
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/test/DurationToMillisTest.kt (Kotlin)

Code before:
    )

    @Test fun one_extra_nanosecond_rounded_up_to_next_millisecond() = assertEquals(
        expected = 999L,
        actual = (998.milliseconds + 1.nanoseconds).toDelayMillis(),
    )

    @Test fun infinite_duration_coerced_to_Long_MAX_VALUE() = assertEquals(
        expected = Long.MAX_VALUE,
        actual = Duration.INFINITE.toDelayMillis(),
    )

    @Test fun negative_infinite_duration_coerced_to_zero() = assertEquals(
        expected = 0L,
        actual = (-Duration.INFINITE).toDelayMillis(),
    )

    @Test fun nanosecond_off_by_one_infinity_does_not_overflow() = assertEquals(
        expected = Long.MAX_VALUE / 1_000_000,
        actual = (Long.MAX_VALUE - 1L).nanoseconds.toDelayMillis(),
    )

    @Test fun millisecond_off_by_one_infinity_does_not_overflow() = assertEquals(
        expected = Long.MAX_VALUE, // INFINITY duration.
        actual = (Long.MAX_VALUE - 1L).milliseconds.toDelayMillis(),
    )

    @Test fun out_of_bounds_nanoseconds_but_finite() {
        val milliseconds = Long.MAX_VALUE / 10
        assertEquals(
            expected = milliseconds,
            actual = milliseconds.milliseconds.toDelayMillis(),
        )
    }
}


Review comment (suggestion):
I actually don't think this is correct now that I look at it again. This duration has no nanosecond sub component and therefore shouldn't be rounded. 

Code after:
    }

    @Test
    fun testOneExtraNanosecondRoundedUpToNextMillisecond() {
        assertEquals(999L, (998.milliseconds + 1.nanoseconds).toDelayMillis())
    }

    @Test
    fun testInfiniteDurationCoercedToLongMaxValue() {
        assertEquals(Long.MAX_VALUE, Duration.INFINITE.toDelayMillis())
    }

    @Test
    fun testNegativeInfiniteDurationCoercedToZero() {
        assertEquals(0L, (-Duration.INFINITE).toDelayMillis())
    }

    @Test
    fun testNanosecondOffByOneInfinityDoesNotOverflow() {
        assertEquals(Long.MAX_VALUE / 1_000_000, (Long.MAX_VALUE - 1L).nanoseconds.toDelayMillis())
    }

    @Test
    fun testMillisecondOffByOneInfinityDoesNotIncrement() {
        assertEquals((Long.MAX_VALUE / 2) - 1, ((Long.MAX_VALUE / 2) - 1).milliseconds.toDelayMillis())
    }

    @Test
    fun testOutOfBoundsNanosecondsButFiniteDoesNotIncrement() {
        val milliseconds = Long.MAX_VALUE / 10
        assertEquals(milliseconds, milliseconds.milliseconds.toDelayMillis())
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/test/DurationToMillisTest.kt (Kotlin)

Code before:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */
package kotlinx.coroutines

import kotlin.test.*
import kotlin.time.*
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.nanoseconds
import kotlin.time.Duration.Companion.seconds

class DurationToMillisTest {

    @Test fun negative_duration_coerced_to_zero_millis() = assertEquals(
        expected = 0L,
        actual = (-1).seconds.toDelayMillis(),
    )

    @Test fun zero_duration_coerced_to_zero_millis() = assertEquals(
        expected = 0L,
        actual = 0.seconds.toDelayMillis(),
    )

    @Test fun one_nanosecond_coerced_to_one_millisecond() = assertEquals(
        expected = 1L,
        actual = 1.nanoseconds.toDelayMillis(),
    )

    @Test fun one_second_coerced_to_1000_milliseconds() = assertEquals(
        expected = 1_000L,
        actual = 1.seconds.toDelayMillis(),
    )

    @Test fun mixed_component_duration_rounded_up_to_next_millisecond() = assertEquals(
        expected = 999L,
        actual = (998.milliseconds + 75909.nanoseconds).toDelayMillis(),
    )

    @Test fun one_extra_nanosecond_rounded_up_to_next_millisecond() = assertEquals(
        expected = 999L,
        actual = (998.milliseconds + 1.nanoseconds).toDelayMillis(),
    )

Review comment (suggestion):
Style: please look at how all the other tests are structured in this library.
* Test names: we use camel case, and also, all test names begin with `test`.
* Indentation is different: `@Test` should be on a separate line, function calls are typically on a single line, etc.
* Explicit parameter names here don't add much and are mostly noise.

Code after:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */
package kotlinx.coroutines

import kotlin.test.*
import kotlin.time.*
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.nanoseconds
import kotlin.time.Duration.Companion.seconds

class DurationToMillisTest {

    @Test
    fun testNegativeDurationCoercedToZeroMillis() {
        assertEquals(0L, (-1).seconds.toDelayMillis())
    }

    @Test
    fun testZeroDurationCoercedToZeroMillis() {
        assertEquals(0L, 0.seconds.toDelayMillis())
    }

    @Test
    fun testOneNanosecondCoercedToOneMillisecond() {
        assertEquals(1L, 1.nanoseconds.toDelayMillis())
    }

    @Test
    fun testOneSecondCoercedTo1000Milliseconds() {
        assertEquals(1_000L, 1.seconds.toDelayMillis())
    }

    @Test
    fun testMixedComponentDurationRoundedUpToNextMillisecond() {
        assertEquals(999L, (998.milliseconds + 75909.nanoseconds).toDelayMillis())
    }

    @Test
    fun testOneExtraNanosecondRoundedUpToNextMillisecond() {
        assertEquals(999L, (998.milliseconds + 1.nanoseconds).toDelayMillis())
    }
Repository: Kotlin/kotlinx.coroutines
File: buildSrc/src/main/kotlin/VersionFile.kt (Kotlin)

Code before:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import org.gradle.api.*
import org.gradle.api.tasks.*
import org.gradle.kotlin.dsl.*

object VersionFile {
    fun configure(project: Project, jarTask: AbstractCopyTask) = with(project) {
        val versionFileTask by tasks.register("versionFileTask") {
            val name = project.name.replace('-', '_')
            val versionFile = project.layout.buildDirectory.file("$name.version")
            outputs.file(versionFile)
            doLast {
                versionFile.get().asFile.writeText(project.version.toString())
            }
        }
        jarTask.dependsOn(versionFileTask)
        jarTask.from(versionFileTask) {
            into("META-INF")
        }
    }
}


Review comment (suggestion):
```suggestion
/**
 * Adds 'module_name.version' file to the project's JAR META-INF
 * for the better toolability. See #2941
 */
object VersionFile {
```

Code after:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import org.gradle.api.*
import org.gradle.api.tasks.*

/**
 * Adds 'module_name.version' file to the project's JAR META-INF
 * for the better toolability. See #2941
 */
object VersionFile {
    fun registerVersionFileTask(project: Project): TaskProvider<Task> {
        val versionFile = project.layout.buildDirectory.file("${project.name.replace('-', '_')}.version")
        val version = project.version.toString()
        return project.tasks.register("versionFileTask") {
            outputs.file(versionFile)
            doLast {
                versionFile.get().asFile.writeText(version)
            }
        }
    }

    fun fromVersionFile(target: AbstractCopyTask, versionFileTask: TaskProvider<Task>) {
        target.from(versionFileTask) {
            into("META-INF")
        }
    }
}
Repository: Kotlin/kotlinx.coroutines
File: buildSrc/src/main/kotlin/VersionFile.kt (Kotlin)

Code before:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import org.gradle.api.*
import org.gradle.api.tasks.*

/**
 * Adds 'module_name.version' file to the project's JAR META-INF
 * for the better toolability. See #2941
 */
object VersionFile {
    fun registerVersionFileTask(project: Project): TaskProvider<Task> = with(project) {
        tasks.register("versionFileTask") {
            val name = project.name.replace('-', '_')
            val versionFile = project.layout.buildDirectory.file("$name.version")
            outputs.file(versionFile)
            doLast {
                versionFile.get().asFile.writeText(project.version.toString())
            }
        }
    }

    fun fromVersionFile(target: AbstractCopyTask, versionFileTask: TaskProvider<Task>) {
        target.from(versionFileTask) {
            into("META-INF")
        }
    }
}


Review comment (performance):
Use of `project` in task execution phase breaks configuration cache compatibility.
Assign the `version` to a variable outside of the `doLast` block to avoid that, just like you did with `name`.

Probably, `with(project)` will also break it (unsure), it's redundant here your could to `project.tasks.register`.

Code after:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import org.gradle.api.*
import org.gradle.api.tasks.*

/**
 * Adds 'module_name.version' file to the project's JAR META-INF
 * for the better toolability. See #2941
 */
object VersionFile {
    fun registerVersionFileTask(project: Project): TaskProvider<Task> {
        val versionFile = project.layout.buildDirectory.file("${project.name.replace('-', '_')}.version")
        val version = project.version.toString()
        return project.tasks.register("versionFileTask") {
            outputs.file(versionFile)
            doLast {
                versionFile.get().asFile.writeText(version)
            }
        }
    }

    fun fromVersionFile(target: AbstractCopyTask, versionFileTask: TaskProvider<Task>) {
        target.from(versionFileTask) {
            into("META-INF")
        }
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-debug/build.gradle.kts (Kotlin)

Code before:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import com.github.jengelman.gradle.plugins.shadow.tasks.*

apply(plugin = "com.github.johnrengelman.shadow")

// apply plugin to use autocomplete for Kover DSL
apply(plugin = "org.jetbrains.kotlinx.kover")

configurations {
    val shadowDeps by creating // shaded dependencies, not included into the resulting .pom file
    compileOnly.get().extendsFrom(shadowDeps)
    runtimeOnly.get().extendsFrom(shadowDeps)
}

val junit_version by properties
val junit5_version by properties
val byte_buddy_version by properties
val blockhound_version by properties
val jna_version by properties

dependencies {
    compileOnly("junit:junit:$junit_version")
    compileOnly("org.junit.jupiter:junit-jupiter-api:$junit5_version")
    testImplementation("org.junit.jupiter:junit-jupiter-engine:$junit5_version")
    testImplementation("org.junit.platform:junit-platform-testkit:1.7.0")
    add("shadowDeps", "net.bytebuddy:byte-buddy:$byte_buddy_version")
    add("shadowDeps", "net.bytebuddy:byte-buddy-agent:$byte_buddy_version")
    compileOnly("io.projectreactor.tools:blockhound:$blockhound_version")
    testImplementation("io.projectreactor.tools:blockhound:$blockhound_version")
    testImplementation("com.google.code.gson:gson:2.8.6")
    api("net.java.dev.jna:jna:$jna_version")
    api("net.java.dev.jna:jna-platform:$jna_version")

Review comment (suggestion):
```suggestion
plugins {
   id("com.github.johnrengelman.shadow")
   id("org.jetbrains.kotlinx.kover") // apply plugin to use autocomplete for Kover DSL
}
```

I don't remember where I read this in the docs, but if I recall correctly, configuration optimisation can be done only if plugins are declared in the newer `plugins` block, instead of legacy `apply` block.

Code after:
/*
 * Copyright 2016-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
 */

import com.github.jengelman.gradle.plugins.shadow.tasks.*
import java.net.*
import java.nio.file.*

plugins {
    id("com.github.johnrengelman.shadow")
    id("org.jetbrains.kotlinx.kover") // apply plugin to use autocomplete for Kover DSL
}

configurations {
    val shadowDeps by creating
    compileOnly.configure {
        extendsFrom(shadowDeps)
    }
    runtimeOnly.configure {
        extendsFrom(shadowDeps)
    }
}

val junit_version by properties
val junit5_version by properties
val byte_buddy_version by properties
val blockhound_version by properties
val jna_version by properties

dependencies {
    compileOnly("junit:junit:$junit_version")
    compileOnly("org.junit.jupiter:junit-jupiter-api:$junit5_version")
    testImplementation("org.junit.jupiter:junit-jupiter-engine:$junit5_version")
    testImplementation("org.junit.platform:junit-platform-testkit:1.7.0")
    add("shadowDeps", "net.bytebuddy:byte-buddy:$byte_buddy_version")
Repository: Kotlin/kotlinx.coroutines
File: buildSrc/src/main/kotlin/version-file-conventions.gradle.kts (Kotlin)

Code before:
import org.gradle.api.tasks.bundling.*

/* `kotlinx-coroutines-debug` configures `VersionFile` on its own when the corresponding task is created. */
val invalidModules = listOf("kotlinx-coroutines-debug")

configure(subprojects.filter {
    !unpublished.contains(it.name) && !invalidModules.contains(it.name) && it.name !in sourceless
}) {
    val project = this
    val jarTaskName = if (isMultiplatform) "jvmJar" else "jar"
    val versionFileTask = VersionFile.registerVersionFileTask(project)
    tasks.withType(Jar::class.java).configureEach {
        if (name == jarTaskName) {
            VersionFile.fromVersionFile(this, versionFileTask)
        }
    }
}


Review comment (suggestion):
Configuring subproject is usually a bad sign.

Instead, you should probably add this configuration logic to your convention plugin, and apply the convention plugin to all relevant subprojects using the `plugins` block.
Gradle recommend that approach as it allow more optimisations. Also, more clarity.
See [that part of the docs](https://docs.gradle.org/current/userguide/sharing_build_logic_between_subprojects.html#sec:convention_plugins_vs_cross_configuration).

You are already using a convention plugin, you just need to use it fully. (I can detail if you need)

Code after:
import org.gradle.api.tasks.bundling.*

configure(subprojects.filter { !unpublished.contains(it.name) && it.name !in sourceless }) {
    val project = this
    val jarTaskName = when {
        project.name == "kotlinx-coroutines-debug" -> {
            project.apply(plugin = "com.github.johnrengelman.shadow")
            "shadowJar"
        }
        isMultiplatform -> "jvmJar"
        else -> "jar"
    }
    val versionFileTask = VersionFile.registerVersionFileTask(project)
    tasks.withType(Jar::class.java).named(jarTaskName) {
        VersionFile.fromVersionFile(this, versionFileTask)
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt (Kotlin)

Code before:
package kotlinx.coroutines.internal

internal expect class ReentrantLock() {
    fun tryLock(): Boolean
    fun unlock()
}

internal expect inline fun <T> ReentrantLock.withLock(action: () -> T): T

internal expect fun <E> identitySet(expectedSize: Int): MutableSet<E>

/**
 * Annotation indicating that the marked property is the subject of benign data race.
 * LLVM does not support this notion, so on K/N platforms we alias it into `@Volatile` to prevent potential OoTA.
 *
 * The purpose of this annotation is not to save an extra-volatile on JVM platform, but rather to explicitly emphasize
 * that data-race is benign.
 */
@OptionalExpectation
@Target(AnnotationTarget.FIELD)
internal expect annotation class BenignDataRace()

// Used **only** as a workaround for #3820 in StateFlow. Do not use anywhere else
internal expect class AtomicReference<T>(value: T) {
    public fun get(): T
    public fun set(value: T)
    public fun getAndSet(value: T): T
    public fun compareAndSet(expected: T, value: T): Boolean
}

@Suppress("UNUSED_PARAMETER", "EXTENSION_SHADOWED_BY_MEMBER")
internal var <T> AtomicReference<T>.value: T
    get() = this.get()
    set(value) = this.set(value)

internal inline fun <T> AtomicReference<T>.loop(action: AtomicReference<T>.(value: T) -> Unit) {
    while (true) {
        action(value)
    }
}


Review comment (suggestion):
`@OptIn(StopDoNotUse::class)`? `class BadAtomicReferenceForWorkarounds`? An arbitrary version of me down the line can't be trusted not to use this `AtomicReference` accidentally, and I don't read non-KDoc comments above every entity I use.

Code after:
package kotlinx.coroutines.internal

internal expect class ReentrantLock() {
    fun tryLock(): Boolean
    fun unlock()
}

internal expect inline fun <T> ReentrantLock.withLock(action: () -> T): T

internal expect fun <E> identitySet(expectedSize: Int): MutableSet<E>

/**
 * Annotation indicating that the marked property is the subject of benign data race.
 * LLVM does not support this notion, so on K/N platforms we alias it into `@Volatile` to prevent potential OoTA.
 *
 * The purpose of this annotation is not to save an extra-volatile on JVM platform, but rather to explicitly emphasize
 * that data-race is benign.
 */
@OptionalExpectation
@Target(AnnotationTarget.FIELD)
internal expect annotation class BenignDataRace()

// Used **only** as a workaround for #3820 in StateFlow. Do not use anywhere else
internal expect class WorkaroundAtomicReference<T>(value: T) {
    public fun get(): T
    public fun set(value: T)
    public fun getAndSet(value: T): T
    public fun compareAndSet(expected: T, value: T): Boolean
}

@Suppress("UNUSED_PARAMETER", "EXTENSION_SHADOWED_BY_MEMBER")
internal var <T> WorkaroundAtomicReference<T>.value: T
    get() = this.get()
    set(value) = this.set(value)

internal inline fun <T> WorkaroundAtomicReference<T>.loop(action: WorkaroundAtomicReference<T>.(value: T) -> Unit) {
    while (true) {
        action(value)
    }
}