Address the review

Author: dkhalanskyjb

core/common/src/Instant.kt

@@ -535,11 +535,3 @@ public fun Instant.Companion.parse(input: CharSequence, format: DateTimeFormat<D
 
 internal const val DISTANT_PAST_SECONDS = -3217862419201
 internal const val DISTANT_FUTURE_SECONDS = 3093527980800
-
-/**
- * Displays the given Instant in the given [offset].
- *
- * Be careful: this function may throw for some values of the [Instant].
- */
-internal fun Instant.toStringWithOffset(offset: UtcOffset): String =
-    format(DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET, offset)

core/common/src/format/DateTimeComponents.kt

@@ -11,7 +11,6 @@ import kotlinx.datetime.internal.*
 import kotlinx.datetime.internal.format.*
 import kotlinx.datetime.internal.format.parser.Copyable
 import kotlinx.datetime.internal.safeMultiply
-import kotlin.native.concurrent.*
 import kotlin.reflect.*
 
 /**
@@ -24,10 +23,10 @@ import kotlin.reflect.*
  * Example:
  * ```
  * val input = "2020-03-16T23:59:59.999999999+03:00"
- * val bag = DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET.parse(input)
- * val localDateTime = bag.toLocalDateTime() // LocalDateTime(2020, 3, 16, 23, 59, 59, 999_999_999)
- * val instant = bag.toInstantUsingOffset() // Instant.parse("2020-03-16T20:59:59.999999999Z")
- * val offset = bag.toUtcOffset() // UtcOffset(hours = 3)
+ * val components = DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET.parse(input)
+ * val localDateTime = components.toLocalDateTime() // LocalDateTime(2020, 3, 16, 23, 59, 59, 999_999_999)
+ * val instant = components.toInstantUsingOffset() // Instant.parse("2020-03-16T20:59:59.999999999Z")
+ * val offset = components.toUtcOffset() // UtcOffset(hours = 3)
  * ```
  *
  * Another purpose is to support parsing and formatting data with out-of-bounds values. For example, parsing
@@ -168,7 +167,7 @@ public class DateTimeComponents internal constructor(internal val contents: Date
 
     /**
      * Writes the contents of the specified [localTime] to this [DateTimeComponents].
-     * The [localTime] is written to the [hour], [hourOfAmPm], [isPm], [minute], [second] and [nanosecond] fields.
+     * The [localTime] is written to the [hour], [hourOfAmPm], [amPm], [minute], [second] and [nanosecond] fields.
      *
      * If any of the fields are already set, they will be overwritten.
      */
@@ -186,7 +185,7 @@ public class DateTimeComponents internal constructor(internal val contents: Date
      * Writes the contents of the specified [localDateTime] to this [DateTimeComponents].
      * The [localDateTime] is written to the
      * [year], [monthNumber], [dayOfMonth], [dayOfWeek],
-     * [hour], [hourOfAmPm], [isPm], [minute], [second] and [nanosecond] fields.
+     * [hour], [hourOfAmPm], [amPm], [minute], [second] and [nanosecond] fields.
      *
      * If any of the fields are already set, they will be overwritten.
      */
@@ -247,7 +246,10 @@ public class DateTimeComponents internal constructor(internal val contents: Date
      */
     public var monthNumber: Int? by TwoDigitNumber(contents.date::monthNumber)
 
-    /** The month ([Month]) component of the date. */
+    /**
+     * The month ([Month]) component of the date.
+     * @throws IllegalArgumentException during getting if [monthNumber] is outside the `1..12` range.
+     */
     public var month: Month?
         get() = monthNumber?.let { Month(it) }
         set(value) {
@@ -270,23 +272,23 @@ public class DateTimeComponents internal constructor(internal val contents: Date
     // public var dayOfYear: Int
 
     /**
-     * The hour-of-day time component.
+     * The hour-of-day (0..23) time component.
      * @throws IllegalArgumentException during assignment if the value is outside the `0..99` range.
      */
     public var hour: Int? by TwoDigitNumber(contents.time::hour)
 
     /**
-     * The 12-hour time component.
+     * The 12-hour (1..12) time component.
      * @throws IllegalArgumentException during assignment if the value is outside the `0..99` range.
-     * @see isPm
+     * @see amPm
      */
     public var hourOfAmPm: Int? by TwoDigitNumber(contents.time::hourOfAmPm)
 
     /**
-     * The AM/PM state of the time component: `true` if PM, `false` if `AM`.
+     * The AM/PM state of the time component.
      * @see hourOfAmPm
      */
-    public var isPm: Boolean? by contents.time::isPm
+    public var amPm: AmPmMarker? by contents.time::amPm
 
     /**
      * The minute-of-hour component.
@@ -368,13 +370,13 @@ public class DateTimeComponents internal constructor(internal val contents: Date
      * Builds a [LocalTime] from the fields in this [DateTimeComponents].
      *
      * This method uses the following fields:
-     * * [hour], [hourOfAmPm], and [isPm]
+     * * [hour], [hourOfAmPm], and [amPm]
      * * [minute]
      * * [second] (default value is 0)
      * * [nanosecond] (default value is 0)
      *
      * @throws IllegalArgumentException if hours or minutes are not present, if any of the fields are invalid, or
-     * [hourOfAmPm] and [isPm] are inconsistent with [hour].
+     * [hourOfAmPm] and [amPm] are inconsistent with [hour].
      */
     public fun toLocalTime(): LocalTime = contents.time.toLocalTime()
 
@@ -385,7 +387,7 @@ public class DateTimeComponents internal constructor(internal val contents: Date
      * * [year]
      * * [monthNumber]
      * * [dayOfMonth]
-     * * [hour], [hourOfAmPm], and [isPm]
+     * * [hour], [hourOfAmPm], and [amPm]
      * * [minute]
      * * [second] (default value is 0)
      * * [nanosecond] (default value is 0)
@@ -421,7 +423,7 @@ public class DateTimeComponents internal constructor(internal val contents: Date
          * * and is a multiple of 400, which is the number of years in a leap cycle, which means that taking the
          *   remainder of the year after dividing by 40_000 will not change the leap year status of the year.
          */
-        truncatedDate.year = getParsedField(truncatedDate.year, "year") % 10_000
+        truncatedDate.year = requireParsedField(truncatedDate.year, "year") % 10_000
         val totalSeconds = try {
             val secDelta = safeMultiply((year!! / 10_000).toLong(), SECONDS_PER_10000_YEARS)
             val epochDays = truncatedDate.toLocalDate().toEpochDays().toLong()
@@ -537,4 +539,4 @@ private class TwoDigitNumber(private val reference: KMutableProperty0<Int?>) {
     }
 }
 
-private val emptyDateTimeComponentsContents = DateTimeComponentsContents()
+private val emptyDateTimeComponentsContents = DateTimeComponentsContents()

core/common/src/format/DateTimeFormat.kt

@@ -8,7 +8,6 @@ package kotlinx.datetime.format
 import kotlinx.datetime.*
 import kotlinx.datetime.internal.format.*
 import kotlinx.datetime.internal.format.parser.*
-import kotlin.native.concurrent.*
 
 /**
  * A format for parsing and formatting date-time-related values.
@@ -46,11 +45,8 @@ public sealed interface DateTimeFormat<T> {
          * The typical use case for this is to create a [DateTimeFormat] instance using a non-idiomatic approach and
          * then convert it to a builder DSL.
          */
-        public fun formatAsKotlinBuilderDsl(format: DateTimeFormat<*>): String {
-            when (format) {
-                is AbstractDateTimeFormat<*, *> -> return format.actualFormat.builderString(allFormatConstants)
-                else -> error("Unsupported format: $format")
-            }
+        public fun formatAsKotlinBuilderDsl(format: DateTimeFormat<*>): String = when (format) {
+            is AbstractDateTimeFormat<*, *> -> format.actualFormat.builderString(allFormatConstants)
         }
     }
 }

core/common/src/format/DateTimeFormatBuilder.kt

@@ -281,8 +281,8 @@ public sealed interface DateTimeFormatBuilder {
  * When formatting, the decimal fraction will round the number to fit in the specified [maxLength] and will add
  * trailing zeroes to the specified [minLength].
  *
- * Additionally, [grouping] is a list, where the i'th element specifies how many trailing zeros to add during formatting
- * when
+ * Additionally, [grouping] is a list, where the i'th (1-based) element specifies how many trailing zeros to add during
+ * formatting when the number would have i digits.
  *
  * When parsing, the parser will require that the fraction is at least [minLength] and at most [maxLength]
  * digits long.
@@ -390,15 +390,6 @@ internal interface AbstractDateTimeFormatBuilder<Target, ActualSelf> :
 
     override fun chars(value: String) = actualBuilder.add(ConstantFormatStructure(value))
 
-    fun withSharedSignImpl(outputPlus: Boolean, block: ActualSelf.() -> Unit) {
-        actualBuilder.add(
-            SignedFormatStructure(
-                createEmpty().also { block(it) }.actualBuilder.build(),
-                outputPlus
-            )
-        )
-    }
-
     fun build(): StringFormat<Target> = StringFormat(actualBuilder.build())
 }
 

core/common/src/format/LocalDateFormat.kt

@@ -9,7 +9,6 @@ import kotlinx.datetime.*
 import kotlinx.datetime.internal.*
 import kotlinx.datetime.internal.format.*
 import kotlinx.datetime.internal.format.parser.Copyable
-import kotlin.native.concurrent.*
 
 /**
  * A description of how month names are formatted.
@@ -116,7 +115,7 @@ internal fun DayOfWeekNames.toKotlinCode(): String = when (this.names) {
     else -> names.joinToString(", ", "DayOfWeekNames(", ")", transform = String::toKotlinCode)
 }
 
-internal fun <T> getParsedField(field: T?, name: String): T {
+internal fun <T> requireParsedField(field: T?, name: String): T {
     if (field == null) {
         throw DateTimeFormatException("Can not create a $name from the given input: the field $name is missing")
     }
@@ -148,9 +147,9 @@ internal class IncompleteLocalDate(
 ) : DateFieldContainer, Copyable<IncompleteLocalDate> {
     fun toLocalDate(): LocalDate {
         val date = LocalDate(
-            getParsedField(year, "year"),
-            getParsedField(monthNumber, "monthNumber"),
-            getParsedField(dayOfMonth, "dayOfMonth")
+            requireParsedField(year, "year"),
+            requireParsedField(monthNumber, "monthNumber"),
+            requireParsedField(dayOfMonth, "dayOfMonth")
         )
         isoDayOfWeek?.let {
             if (it != date.dayOfWeek.isoDayNumber) {

core/common/src/format/LocalTimeFormat.kt

@@ -9,15 +9,24 @@ import kotlinx.datetime.*
 import kotlinx.datetime.internal.*
 import kotlinx.datetime.internal.format.*
 import kotlinx.datetime.internal.format.parser.Copyable
-import kotlin.native.concurrent.*
+
+/**
+ * The AM/PM marker that indicates whether the hour in range `1..12` is before or after noon.
+ */
+public enum class AmPmMarker {
+    /** The time is before noon. */
+    AM,
+    /** The time is after noon. */
+    PM,
+}
 
 internal interface TimeFieldContainer {
     var minute: Int?
     var second: Int?
     var nanosecond: Int?
     var hour: Int?
     var hourOfAmPm: Int?
-    var isPm: Boolean?
+    var amPm: AmPmMarker?
 
     var fractionOfSecond: DecimalFraction?
         get() = nanosecond?.let { DecimalFraction(it, 9) }
@@ -31,14 +40,14 @@ private object TimeFields {
     val minute = UnsignedFieldSpec(PropertyAccessor(TimeFieldContainer::minute), minValue = 0, maxValue = 59)
     val second = UnsignedFieldSpec(PropertyAccessor(TimeFieldContainer::second), minValue = 0, maxValue = 59, defaultValue = 0)
     val fractionOfSecond = GenericFieldSpec(PropertyAccessor(TimeFieldContainer::fractionOfSecond), defaultValue = DecimalFraction(0, 9))
-    val isPm = GenericFieldSpec(PropertyAccessor(TimeFieldContainer::isPm))
+    val amPm = GenericFieldSpec(PropertyAccessor(TimeFieldContainer::amPm))
     val hourOfAmPm = UnsignedFieldSpec(PropertyAccessor(TimeFieldContainer::hourOfAmPm), minValue = 1, maxValue = 12)
 }
 
 internal class IncompleteLocalTime(
     override var hour: Int? = null,
     override var hourOfAmPm: Int? = null,
-    override var isPm: Boolean? = null,
+    override var amPm: AmPmMarker? = null,
     override var minute: Int? = null,
     override var second: Int? = null,
     override var nanosecond: Int? = null
@@ -48,20 +57,20 @@ internal class IncompleteLocalTime(
             hourOfAmPm?.let {
                 require((hour + 11) % 12 + 1 == it) { "Inconsistent hour and hour-of-am-pm: hour is $hour, but hour-of-am-pm is $it" }
             }
-            isPm?.let {
-                require(isPm != null && isPm != (hour >= 12)) {
-                    "Inconsistent hour and the AM/PM marker: hour is $hour, but the AM/PM marker is ${if (it) "PM" else "AM"}"
+            amPm?.let { amPm ->
+                require((amPm == AmPmMarker.PM) == (hour >= 12)) {
+                    "Inconsistent hour and the AM/PM marker: hour is $hour, but the AM/PM marker is $amPm"
                 }
             }
             hour
         } ?: hourOfAmPm?.let { hourOfAmPm ->
-            isPm?.let { isPm ->
-                hourOfAmPm.let { if (it == 12) 0 else it } + if (isPm) 12 else 0
+            amPm?.let { amPm ->
+                hourOfAmPm.let { if (it == 12) 0 else it } + if (amPm == AmPmMarker.PM) 12 else 0
             }
         } ?: throw DateTimeFormatException("Incomplete time: missing hour")
         return LocalTime(
             hour,
-            getParsedField(minute, "minute"),
+            requireParsedField(minute, "minute"),
             second ?: 0,
             nanosecond ?: 0,
         )
@@ -70,20 +79,20 @@ internal class IncompleteLocalTime(
     fun populateFrom(localTime: LocalTime) {
         hour = localTime.hour
         hourOfAmPm = (localTime.hour + 11) % 12 + 1
-        isPm = localTime.hour >= 12
+        amPm = if (localTime.hour >= 12) AmPmMarker.PM else AmPmMarker.AM
         minute = localTime.minute
         second = localTime.second
         nanosecond = localTime.nanosecond
     }
 
-    override fun copy(): IncompleteLocalTime = IncompleteLocalTime(hour, hourOfAmPm, isPm, minute, second, nanosecond)
+    override fun copy(): IncompleteLocalTime = IncompleteLocalTime(hour, hourOfAmPm, amPm, minute, second, nanosecond)
 
     override fun equals(other: Any?): Boolean =
-        other is IncompleteLocalTime && hour == other.hour && hourOfAmPm == other.hourOfAmPm && isPm == other.isPm &&
+        other is IncompleteLocalTime && hour == other.hour && hourOfAmPm == other.hourOfAmPm && amPm == other.amPm &&
             minute == other.minute && second == other.second && nanosecond == other.nanosecond
 
     override fun hashCode(): Int =
-        (hour ?: 0) * 31 + (hourOfAmPm ?: 0) * 31 + (isPm?.hashCode() ?: 0) * 31 + (minute ?: 0) * 31 +
+        (hour ?: 0) * 31 + (hourOfAmPm ?: 0) * 31 + (amPm?.hashCode() ?: 0) * 31 + (minute ?: 0) * 31 +
             (second ?: 0) * 31 + (nanosecond ?: 0)
 
     override fun toString(): String =
@@ -145,10 +154,10 @@ private class AmPmHourDirective(private val padding: Padding) :
 }
 
 private class AmPmMarkerDirective(private val amString: String, private val pmString: String) :
-    NamedEnumIntFieldFormatDirective<TimeFieldContainer, Boolean>(
-        TimeFields.isPm, mapOf(
-            false to amString,
-            true to pmString,
+    NamedEnumIntFieldFormatDirective<TimeFieldContainer, AmPmMarker>(
+        TimeFields.amPm, mapOf(
+            AmPmMarker.AM to amString,
+            AmPmMarker.PM to pmString,
         ),
         "AM/PM marker"
     ) {

core/common/src/format/migration/Unicode.kt

@@ -9,6 +9,7 @@ import kotlin.native.concurrent.*
 
 /**
  * Marks declarations in the datetime library that use format strings to define datetime formats.
+ *
  * Format strings are discouraged, because they require gaining proficiency in another tiny language.
  * When possible, please use the builder-style Kotlin API instead.
  * If the format string is a constant, the corresponding builder-style Kotlin code can be obtained by calling

core/common/test/InstantTest.kt

@@ -6,13 +6,11 @@
 package kotlinx.datetime.test
 
 import kotlinx.datetime.*
-import kotlinx.datetime.Clock // currently, requires an explicit import due to a conflict with the deprecated Clock from kotlin.time
 import kotlinx.datetime.format.*
 import kotlinx.datetime.internal.*
 import kotlin.random.*
 import kotlin.test.*
 import kotlin.time.*
-import kotlin.time.Duration.Companion.days
 import kotlin.time.Duration.Companion.hours
 import kotlin.time.Duration.Companion.milliseconds
 import kotlin.time.Duration.Companion.nanoseconds
@@ -120,19 +118,24 @@ class InstantTest {
             Instant.DISTANT_PAST,
             Instant.fromEpochSeconds(0, 0))
 
-        val offsets = listOf(
-            UtcOffset.parse("Z"),
-            UtcOffset.parse("+03:12:14"),
-            UtcOffset.parse("-03:12:14"),
-            UtcOffset.parse("+02:35"),
-            UtcOffset.parse("-02:35"),
-            UtcOffset.parse("+04"),
-            UtcOffset.parse("-04"),
+        val offsetStrings = listOf(
+            "Z",
+            "+03:12:14",
+            "-03:12:14",
+            "+02:35",
+            "-02:35",
+            "+04",
+            "-04",
         )
 
+        val offsets = offsetStrings.map { UtcOffset.parse(it) }
+
         for (instant in instants) {
-            for (offset in offsets) {
-                val str = instant.format(DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET, offset)
+            for (offsetIx in offsets.indices) {
+                val str = instant.format(DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET, offsets[offsetIx])
+                val offsetString = offsetStrings[offsetIx]
+                assertEquals(offsetString, offsetString.commonSuffixWith(str))
+                assertEquals(instant, Instant.parse(str, DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET))
                 assertEquals(instant, Instant.parse(str))
             }
         }