ronantakizawa/codereview-bench huggingface dataset:
Repository: square/leakcanary
File: shark-graph/src/main/java/shark/ProguardMapping.kt (Kotlin)

Code before:
package shark

import java.io.File
import java.io.FileNotFoundException
import java.io.IOException
import java.io.InputStream
import java.text.ParseException


class ProguardMapping private constructor(
        proguardMappingFile: InputStream
) {

    // Classes can be simply stored as a obfuscatedName -> clearName
    // For fields it's a bit more complicated since we need to know
    // the class that a given field belongs to (two different classes
    // can have a field with the same name). So files are stored as:
    // obfuscatedClassName.obfuscatedFieldName -> clearFieldName
    private val obfuscatedToClearNamesMap = hashMapOf<String, String>()

    init {
        parseMappingFile(proguardMappingFile)
    }

    /**
     * Returns deobfuscated class name or original string if there is no
     * mapping for given obfuscated name.
     */
    fun deobfuscateClassName(obfuscatedClassName: String): String {
        return obfuscatedToClearNamesMap[obfuscatedClassName] ?: obfuscatedClassName
    }

    /**
     * Returns deobfuscated field name or original string if there is no
     * mapping for given obfuscated name.
     */
    fun deobfuscateFieldName(obfuscatedClass: String, obfuscatedField: String): String {
        return obfuscatedToClearNamesMap["$obfuscatedClass.$obfuscatedField"] ?: return obfuscatedField
    }

    @Throws(FileNotFoundException::class, IOException::class, ParseException::class)
    private fun parseMappingFile(mappingInputStream: InputStream) {
        mappingInputStream.bufferedReader(Charsets.UTF_8).use { bufferedReader ->


Review comment (bug):
Nit: use linked hash map instead to get consistent ordering at minimal cost (useful when debugging)

Code after:
package shark

class ProguardMapping {

  // Classes can be simply stored as a obfuscatedName -> clearName
  // For fields it's a bit more complicated since we need to know
  // the class that a given field belongs to (two different classes
  // can have a field with the same name). So files are stored as:
  // obfuscatedClassName.obfuscatedFieldName -> clearFieldName
  private val obfuscatedToClearNamesMap = linkedMapOf<String, String>()

  /**
   * Returns deobfuscated class name or original string if there is no
   * mapping for given obfuscated name.
   */
  fun deobfuscateClassName(obfuscatedClassName: String): String {
    return obfuscatedToClearNamesMap[obfuscatedClassName] ?: obfuscatedClassName
  }

  /**
   * Returns deobfuscated field name or original string if there is no
   * mapping for given obfuscated name.
   */
  fun deobfuscateFieldName(obfuscatedClass: String, obfuscatedField: String): String {
    return obfuscatedToClearNamesMap["$obfuscatedClass.$obfuscatedField"] ?: return obfuscatedField
  }

  /**
   * Adds entry to the obfuscatedToClearNamesMap map.
   */
  fun addMapping(obfuscatedName: String, clearName: String) {
    obfuscatedToClearNamesMap[obfuscatedName] = clearName
  }
}
Repository: square/leakcanary
File: shark-graph/src/main/java/shark/ProguardMapping.kt (Kotlin)

Code before:
package shark

import java.io.File
import java.io.FileNotFoundException
import java.io.IOException
import java.io.InputStream
import java.text.ParseException


class ProguardMapping private constructor(
        proguardMappingFile: InputStream
) {

    // Classes can be simply stored as a obfuscatedName -> clearName
    // For fields it's a bit more complicated since we need to know
    // the class that a given field belongs to (two different classes
    // can have a field with the same name). So files are stored as:
    // obfuscatedClassName.obfuscatedFieldName -> clearFieldName
    private val obfuscatedToClearNamesMap = hashMapOf<String, String>()

    init {
        parseMappingFile(proguardMappingFile)
    }

    /**
     * Returns deobfuscated class name or original string if there is no
     * mapping for given obfuscated name.
     */
    fun deobfuscateClassName(obfuscatedClassName: String): String {
        return obfuscatedToClearNamesMap[obfuscatedClassName] ?: obfuscatedClassName
    }

    /**
     * Returns deobfuscated field name or original string if there is no
     * mapping for given obfuscated name.

Review comment (suggestion):
Consider having two types, or an interface and an implementation. HprofGraph only needs to know about a map of obfuscated names to deobfuscated names. It's unclear that all usages will need to work of off an input stream.

Code after:
package shark

class ProguardMapping {

  // Classes can be simply stored as a obfuscatedName -> clearName
  // For fields it's a bit more complicated since we need to know
  // the class that a given field belongs to (two different classes
  // can have a field with the same name). So files are stored as:
  // obfuscatedClassName.obfuscatedFieldName -> clearFieldName
  private val obfuscatedToClearNamesMap = linkedMapOf<String, String>()

  /**
   * Returns deobfuscated class name or original string if there is no
   * mapping for given obfuscated name.
   */
  fun deobfuscateClassName(obfuscatedClassName: String): String {
    return obfuscatedToClearNamesMap[obfuscatedClassName] ?: obfuscatedClassName
  }

  /**
   * Returns deobfuscated field name or original string if there is no
   * mapping for given obfuscated name.
   */
  fun deobfuscateFieldName(obfuscatedClass: String, obfuscatedField: String): String {
    return obfuscatedToClearNamesMap["$obfuscatedClass.$obfuscatedField"] ?: return obfuscatedField
  }

  /**
   * Adds entry to the obfuscatedToClearNamesMap map.
   */
  fun addMapping(obfuscatedName: String, clearName: String) {
    obfuscatedToClearNamesMap[obfuscatedName] = clearName
  }
}
Repository: square/leakcanary
File: shark-cli/src/main/java/shark/Main.kt (Kotlin)

Code before:

  return heapDumpFile
}

private fun runCommand(
  directory: File,
  vararg arguments: String
): String {
  val process = ProcessBuilder(*arguments)
      .directory(directory)
      .start()
      .also { it.waitFor(10, SECONDS) }

  if (process.exitValue() != 0) {
    throw Exception(process.errorStream.bufferedReader().readText())
  }
  return process.inputStream.bufferedReader()
      .readText()
}

private fun analyze(heapDumpFile: File, proguardMappingFile: File? = null) {
  val listener = OnAnalysisProgressListener { step ->
    SharkLog.d { step.name }
  }

  val proguardMappingReader = proguardMappingFile?.let { ProguardMappingReader(it.inputStream()) }

  val heapAnalyzer = HeapAnalyzer(listener)
  SharkLog.d { "Analyzing heap dump $heapDumpFile" }
  val heapAnalysis = heapAnalyzer.analyze(
      heapDumpFile, AndroidReferenceMatchers.appDefaults, true,
      AndroidObjectInspectors.appDefaults,
      proguardMappingReader = proguardMappingReader
  )

  SharkLog.d { heapAnalysis.toString() }
}

private fun stripHprof(heapDumpFile: File) {
  SharkLog.d { "Stripping primitive arrays in heap dump $heapDumpFile" }
  val stripper = HprofPrimitiveArrayStripper()
  val outputFile = stripper.stripPrimitiveArrays(heapDumpFile)
  SharkLog.d { "Stripped primitive arrays to $outputFile" }
}

Review comment (suggestion):
Sorry I should have been more clear: I think we should push ProguardMappingReader to the outmost layer. I can't think of a good reason for HeapAnalyzer to know about where the mapping comes from. The general idea is to enable doing this even when we don't have a file

Code after:

  return heapDumpFile
}

private fun runCommand(
  directory: File,
  vararg arguments: String
): String {
  val process = ProcessBuilder(*arguments)
      .directory(directory)
      .start()
      .also { it.waitFor(10, SECONDS) }

  if (process.exitValue() != 0) {
    throw Exception(process.errorStream.bufferedReader().readText())
  }
  return process.inputStream.bufferedReader()
      .readText()
}

private fun analyze(
  heapDumpFile: File,
  proguardMappingFile: File? = null
) {
  val listener = OnAnalysisProgressListener { step ->
    SharkLog.d { step.name }
  }

  val proguardMapping = proguardMappingFile?.let {
    ProguardMappingReader(it.inputStream()).readProguardMapping()
  }

  val heapAnalyzer = HeapAnalyzer(listener)
  SharkLog.d { "Analyzing heap dump $heapDumpFile" }
  val heapAnalysis = heapAnalyzer.analyze(
      heapDumpFile, AndroidReferenceMatchers.appDefaults, true,
      AndroidObjectInspectors.appDefaults,
      proguardMapping = proguardMapping
  )

  SharkLog.d { heapAnalysis.toString() }
}

private fun stripHprof(heapDumpFile: File) {
  SharkLog.d { "Stripping primitive arrays in heap dump $heapDumpFile" }
  val stripper = HprofPrimitiveArrayStripper()
  val outputFile = stripper.stripPrimitiveArrays(heapDumpFile)
  SharkLog.d { "Stripped primitive arrays to $outputFile" }
}
Repository: square/leakcanary
File: shark-log/src/main/java/shark/SharkLog.kt (Kotlin)

Code before:
package shark

/**
 * Central Logger for all Shark artifacts. Set [logger] to change where these logs go.
 */
object SharkLog {

  /**
   * @see SharkLog
   */
  interface Logger {

    /**
     * Logs a debug message formatted with the passed in arguments.
     */
    fun d(message: String)

    /**
     * Logs a [Throwable] and debug message formatted with the passed in arguments.
     */
    fun e(throwable: Throwable, message: String)
  }

  @Volatile var logger: Logger? = null

  /**
   * @see Logger.d
   */
  inline fun d(message: () -> String) {
    // Local variable to prevent the ref from becoming null after the null check.
    val logger = logger ?: return
    logger.d(message.invoke())
  }

  /**
   * @see Logger.e
   */
  inline fun e(throwable: Throwable, message: () -> String) {
    // Local variable to prevent the ref from becoming null after the null check.
    val logger = logger ?: return
    logger.e(throwable, message.invoke())

Review comment (suggestion):
I'm thinking we should remove all method that must take a string, it's way too easy to forget to use the callback one and pass in interpolated strings.

Code after:
package shark

/**
 * Central Logger for all Shark artifacts. Set [logger] to change where these logs go.
 */
object SharkLog {

  /**
   * @see SharkLog
   */
  interface Logger {

    /**
     * Logs a debug message formatted with the passed in arguments.
     */
    fun d(message: String)

    /**
     * Logs a [Throwable] and debug message formatted with the passed in arguments.
     */
    fun d(throwable: Throwable, message: String)
  }

  @Volatile var logger: Logger? = null

  /**
   * @see Logger.d
   */
  inline fun d(message: () -> String) {
    // Local variable to prevent the ref from becoming null after the null check.
    val logger = logger ?: return
    logger.d(message.invoke())
  }

  /**
   * @see Logger.d
   */
  inline fun d(throwable: Throwable, message: () -> String) {
    // Local variable to prevent the ref from becoming null after the null check.
    val logger = logger ?: return
    logger.d(throwable, message.invoke())
Repository: square/leakcanary
File: shark-log/src/test/java/shark/SharkLogTest.kt (Kotlin)

Code before:
import org.junit.Test
import shark.SharkLog.Logger
import java.io.ByteArrayOutputStream
import java.io.PrintStream

class SharkLogTest {

  @Test fun `logging works when logger is set`() {

    val outputStream = ByteArrayOutputStream()
    val printStream = PrintStream(outputStream)

    SharkLog.logger = object : Logger {
      override fun d(message: String) = printStream.print(message)
      override fun e(throwable: Throwable, message: String) = printStream.print("$message+${throwable.message}")
    }

    //Test debug logging
    SharkLog.d { "Test debug" }
    assertThat(outputStream.toString()).isEqualTo("Test debug")

    outputStream.reset()

    //Test error logging
    SharkLog.e(Exception("Test exception")) { "Test error" }
    assertThat(outputStream.toString()).isEqualTo("Test error+Test exception")
  }

  @Test fun `logging is no-op without logger and string is ignored`() {

    SharkLog.logger = null

    //Logging message will throw an exception when attempting to use it
    //But since it's in lambda string will not be accessed
    SharkLog.d { "".substring(1) }

    SharkLog.e(Exception("Test exception")) { "".substring(1) }
  }
}

Review comment (bug):
nit: split these in two distinct test, one for debug and one for error. General guideline is one assert per test.

Code after:
import org.junit.Test
import shark.SharkLog.Logger
import java.io.ByteArrayOutputStream
import java.io.PrintStream

class SharkLogTest {

  private class StreamLogger(private val stream: PrintStream) : Logger {
    override fun d(message: String) = stream.print(message)
    override fun d(throwable: Throwable, message: String) = stream.print("$message ${throwable.message}")
  }

  @Test fun `logging works when logger is set`() {
    val outputStream = ByteArrayOutputStream()

    SharkLog.logger = StreamLogger(PrintStream(outputStream))

    // Test debug logging
    SharkLog.d { "Test debug" }
    assertThat(outputStream.toString()).isEqualTo("Test debug")
  }

  @Test fun `logging with exception works when logger is set`() {
    val outputStream = ByteArrayOutputStream()

    SharkLog.logger = StreamLogger(PrintStream(outputStream))

    // Test error logging
    SharkLog.d(Exception("Test exception")) { "Test error" }
    assertThat(outputStream.toString()).isEqualTo("Test error Test exception")
  }

  @Test fun `logging is no-op without logger and string is ignored`() {
    SharkLog.logger = null

    // Logging message will throw an exception when attempting to use it
    // But since it's in lambda string will not be accessed
    SharkLog.d { "".substring(1) }

    SharkLog.d(Exception("Test exception")) { "".substring(1) }
  }
}
Repository: square/leakcanary
File: shark-graph/src/main/java/shark/HprofHeapGraph.kt (Kotlin)

Code before:
      null
    } else {
      return findObjectById(classId) as HeapClass
    }
  }

  override fun objectExists(objectId: Long): Boolean {
    return index.objectIdIsIndexed(objectId)
  }

  internal fun fieldName(
    classId: Long,
    fieldRecord: FieldRecord
  ): String {
    return index.fieldName(classId, fieldRecord.nameStringId)
  }

  internal fun staticFieldName(
    classId: Long,
    fieldRecord: StaticFieldRecord
  ): String {
    return index.fieldName(classId, fieldRecord.nameStringId)
  }

  internal fun createFieldValuesReader(record: InstanceDumpRecord): FieldValuesReader {
    return FieldValuesReader(record, identifierByteSize)
  }

  internal fun className(classId: Long): String {
    return index.className(classId)
  }

  internal fun readObjectArrayDumpRecord(
    objectId: Long,
    indexedObject: IndexedObjectArray
  ): ObjectArrayDumpRecord {
    return readObjectRecord(objectId, indexedObject) {
      hprof.reader.readObjectArrayDumpRecord()
    }
  }

  internal fun readPrimitiveArrayDumpRecord(
    objectId: Long,
    indexedObject: IndexedPrimitiveArray
  ): PrimitiveArrayDumpRecord {
    return readObjectRecord(objectId, indexedObject) {
      hprof.reader.readPrimitiveArrayDumpRecord()
    }
  }

  internal fun readClassDumpRecord(

Review comment (suggestion):
Nit: can convert to expression body instead of having a `return`

Code after:
      null
    } else {
      return findObjectById(classId) as HeapClass
    }
  }

  override fun objectExists(objectId: Long): Boolean {
    return index.objectIdIsIndexed(objectId)
  }

  internal fun fieldName(
    classId: Long,
    fieldRecord: FieldRecord
  ): String {
    return index.fieldName(classId, fieldRecord.nameStringId)
  }

  internal fun staticFieldName(
    classId: Long,
    fieldRecord: StaticFieldRecord
  ): String {
    return index.fieldName(classId, fieldRecord.nameStringId)
  }

  internal fun createFieldValuesReader(record: InstanceDumpRecord) =
    FieldValuesReader(record, identifierByteSize)

  internal fun className(classId: Long): String {
    return index.className(classId)
  }

  internal fun readObjectArrayDumpRecord(
    objectId: Long,
    indexedObject: IndexedObjectArray
  ): ObjectArrayDumpRecord {
    return readObjectRecord(objectId, indexedObject) {
      hprof.reader.readObjectArrayDumpRecord()
    }
  }

  internal fun readPrimitiveArrayDumpRecord(
    objectId: Long,
    indexedObject: IndexedPrimitiveArray
  ): PrimitiveArrayDumpRecord {
    return readObjectRecord(objectId, indexedObject) {
      hprof.reader.readPrimitiveArrayDumpRecord()
    }
  }

  internal fun readClassDumpRecord(
    objectId: Long,
Repository: square/leakcanary
File: shark-graph/src/main/java/shark/internal/ByteSubArray.kt (Kotlin)

Code before:
package shark.internal

/**
 * Provides read access to a sub part of a larger array.
 */
internal class ByteSubArray(
  private val array: ByteArray,
  private val rangeStart: Int,
  private val rangeEndInclusive: Int,
  private val longIdentifiers: Boolean
) {

  val size = rangeEndInclusive + 1
  private var currentIndex = 0

  fun readByte(): Byte {
    val index = currentIndex
    currentIndex++
    require(index in 0..rangeEndInclusive) {
      "Index $index should be between 0 and $rangeEndInclusive"
    }
    return array[rangeStart + index]
  }

  fun readId(): Long {
    return if (longIdentifiers) {
      readLong()
    } else {
      readInt().toLong()
    }
  }

  fun readInt(): Int {
    val index = currentIndex
    currentIndex += 4
    require(index >= 0 && index <= rangeEndInclusive - 3) {

Review comment (bug):
I'm a bit confused about indexes here, probably I'm understanding it wrong. Here's my thought process:

Originally we created range as `valueIndex until valueIndex + bytesPerValue`, so `endInclusive` was set to `range.endIncusive - range.start = valueIndex + bytesPerValue - valueIndex = bytesPerValue`

With new implementation we pass `rangeEndInclusive` as `(valueIndex + bytesPerValue) - 1` and use it as a replacement for `endInclusive`. 

But, obviously, `(valueIndex + bytesPerValue) - 1 != bytesPerValue`. Is it me misunderstanding the flow and `IntRange` here or is there a bug?

Code after:
package shark.internal

/**
 * Provides read access to a sub part of a larger array.
 */
internal class ByteSubArray(
  private val array: ByteArray,
  private val rangeStart: Int,
  size: Int,
  private val longIdentifiers: Boolean
) {

  private val endInclusive = size - 1

  private var currentIndex = 0

  fun readByte(): Byte {
    val index = currentIndex
    currentIndex++
    require(index in 0..endInclusive) {
      "Index $index should be between 0 and $endInclusive"
    }
    return array[rangeStart + index]
  }

  fun readId(): Long {
    return if (longIdentifiers) {
      readLong()
    } else {
      readInt().toLong()
    }
  }

  fun readInt(): Int {
    val index = currentIndex
    currentIndex += 4
Repository: square/leakcanary
File: shark/src/main/java/shark/ReferencePattern.kt (Kotlin)

Code before:
package shark

import java.io.Serializable

/**
 * A pattern that will match references for a given [ReferenceMatcher].
 */
sealed class ReferencePattern : Serializable {

  /**
   * Matches local references held in the stack of frames of a given thread, identified by its name.
   */
  data class JavaLocalPattern(
    val threadName: String
  ) : ReferencePattern() {
    override fun toString(): String {
      return "local variable on thread $threadName"
    }
  }

  /**
   * Matches static field references, identified by [className] and [fieldName].
   */
  data class StaticFieldPattern(
    val className: String,
    val fieldName: String
  ) : ReferencePattern() {
    override fun toString(): String {
      return "static field $className#$fieldName"
    }
  }

  /**
   * Matches instances field references, identified by [className] and [fieldName].
   *
   * Note: If [fieldName] is declared in a superclass it will still match for subclasses.
   * This is to support overriding of rules for specific cases. If two [ReferenceMatcher] match for
   * the same [fieldName] but for different [className] in a class hierarchy, then the closest
   * class in the hierarchy wins.
   */
  data class InstanceFieldPattern(
    val className: String,
    val fieldName: String
  ) : ReferencePattern() {
    override fun toString(): String {
      return "instance field $className#$fieldName"
    }
  }
}

Review comment (suggestion):
Nit: here and in other classes I think it would be better for readability to go with the expression body:
`override fun toString() = "native global variable referencing $className"`
The strings are short enough to fi within line limit and it will result in nice 3-row data classes

Code after:
  ) : ReferencePattern() {
    override fun toString(): String {
      return "static field $className#$fieldName"
    }
  }

  /**
   * Matches instances field references, identified by [className] and [fieldName].
   *
   * Note: If [fieldName] is declared in a superclass it will still match for subclasses.
   * This is to support overriding of rules for specific cases. If two [ReferenceMatcher] match for
   * the same [fieldName] but for different [className] in a class hierarchy, then the closest
   * class in the hierarchy wins.
   */
  data class InstanceFieldPattern(
    val className: String,
    val fieldName: String
  ) : ReferencePattern() {
    override fun toString(): String {
      return "instance field $className#$fieldName"
    }
  }

  data class NativeGlobalVariablePattern(val className: String) : ReferencePattern() {
    override fun toString(): String {
      return "native global variable referencing $className"
    }
  }
}
Repository: square/leakcanary
File: leakcanary-android-core/src/test/java/leakcanary/LeakCanaryConfigTest.kt (Kotlin)

Code before:
import kotlin.reflect.full.memberFunctions
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    val propertiesInConfig = LeakCanary.Config::class.memberProperties
        .filter { member ->
          // Ignore deprecated fields, we don't need builders for those
          member.annotations.none { it is Deprecated }
        }
        .map { it.name }

    val functionsInConfigBuilder = LeakCanary.Config.Builder::class.memberFunctions
        .map { it.name }


    val missingInConfigBuilder = propertiesInConfig.subtract(functionsInConfigBuilder)
    assertThat(missingInConfigBuilder)
        .withFailMessage("Fields in LeakCanary.Config don't have matching " +
            "builder functions in LeakCanary.Config.Builder: $missingInConfigBuilder")
        .isEmpty()
  }
}

Review comment (bug):
Was considering doing `If (missingInConfigBuilder.isNotEmpty()) { fail ("Error message") }` instead. Let me know if you think it would be cleaner.

Code after:
package leakcanary

import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import kotlin.reflect.full.memberFunctions
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    assertThat(configProperties())
        .containsExactlyInAnyOrderElementsOf(configBuilderFunctions())
  }

  private fun configBuilderFunctions() = LeakCanary.Config.Builder::class.memberFunctions
      .map { it.name }
      .subtract(listOf("build", "equals", "hashCode", "toString"))

  private fun configProperties() = LeakCanary.Config::class.memberProperties
      .filter { member ->
        // Ignore deprecated fields, we don't need builders for those
        member.annotations.none { it is Deprecated }
      }
      .map { it.name }
}