Skip to content

Commit 35fbb01

Browse files
BurntSushiBurntSushi
committed
util: stand up an alternative approach to integer range checking
This is basically an admission that my ranged integer abstraction was a failure. It was legitimately useful in the beginning when I was writing the core date algorithms, but it has since been mostly an annoyance and a hindrance. I could have switched to a stricter and less ergonomic approach (like `deranged`) instead of my Ada-inspired abstraction. But I felt like this would still overall be quite annoying and would inspire a bunch of parallel-but-crate-internal APIs that operate on the special range types internally. That's what Jiff has now, and it's supremely annoying. This is just the first step toward ripping out ranged integers. In particular, this includes a more lightweight abstraction for range checking, and moves `jiff::fmt::strtime` off of its own use of ranged integers. The major bummer here is that the range checking is not reflected in the type system anywhere. e.g., Instead of using a special `Year` type to represent a year value, we just use an `i16` and rely on checking its range at the boundaries of encapsulation. This doesn't change anything for callers, but it does mean that Jiff may do redundant range checking unless the compiler optimizes it out. This makes me sad, but at this point, I think it's still worth ripping ranged integers out and maybe trying to eliminate redundant range checking some other way. But so far, there are no perf regressions in `strftime` or `strptime` benchmarks.
1 parent bd32085 commit 35fbb01

File tree

11 files changed

+557
-350
lines changed

11 files changed

+557
-350
lines changed

‎src/error/mod.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::util::sync::Arc;
1+
use crate::util::{b::BoundsError, sync::Arc};
22

33
pub(crate) mod civil;
44
pub(crate) mod duration;
@@ -232,6 +232,12 @@ impl Error {
232232
Error::from(ErrorKind::SlimRange(SlimRangeError::new(what)))
233233
}
234234

235+
#[inline(never)]
236+
#[cold]
237+
pub(crate) fn bounds(err: BoundsError) -> Error {
238+
Error::from(ErrorKind::Bounds(err))
239+
}
240+
235241
/// Creates a new error from the special "shared" error type.
236242
pub(crate) fn itime_range(
237243
err: crate::shared::util::itime::RangeError,
@@ -421,6 +427,7 @@ impl core::fmt::Debug for Error {
421427
#[cfg_attr(not(feature = "alloc"), derive(Clone))]
422428
enum ErrorKind {
423429
Adhoc(AdhocError),
430+
Bounds(BoundsError),
424431
Civil(self::civil::Error),
425432
CrateFeature(CrateFeatureError),
426433
Duration(self::duration::Error),
@@ -470,6 +477,7 @@ impl core::fmt::Display for ErrorKind {
470477

471478
match *self {
472479
Adhoc(ref msg) => msg.fmt(f),
480+
Bounds(ref msg) => msg.fmt(f),
473481
Civil(ref err) => err.fmt(f),
474482
CrateFeature(ref err) => err.fmt(f),
475483
Duration(ref err) => err.fmt(f),

‎src/fmt/friendly/parser.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::{
55
util::{parse_temporal_fraction, DurationUnits},
66
Parsed,
77
},
8-
util::{c::Sign, parse},
8+
util::{b::Sign, parse},
99
Error, SignedDuration, Span, Unit,
1010
};
1111

‎src/fmt/strtime/format.rs‎

Lines changed: 44 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -368,8 +368,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
368368
let meridiem = match self.tm.meridiem() {
369369
Some(meridiem) => meridiem,
370370
None => {
371-
let hour =
372-
self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
371+
let hour = self.tm.hour().ok_or(FE::RequiresTime)?;
373372
if hour < 12 {
374373
Meridiem::AM
375374
} else {
@@ -389,8 +388,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
389388
let meridiem = match self.tm.meridiem() {
390389
Some(meridiem) => meridiem,
391390
None => {
392-
let hour =
393-
self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
391+
let hour = self.tm.hour().ok_or(FE::RequiresTime)?;
394392
if hour < 12 {
395393
Meridiem::AM
396394
} else {
@@ -472,27 +470,25 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
472470
fn fmt_day_zero(&self) -> Result<ItemInteger, Error> {
473471
let day = self
474472
.tm
475-
.day
476-
.or_else(|| self.tm.to_date().ok().map(|d| d.day_ranged()))
477-
.ok_or(FE::RequiresDate)?
478-
.get();
473+
.day()
474+
.or_else(|| self.tm.to_date().ok().map(|d| d.day()))
475+
.ok_or(FE::RequiresDate)?;
479476
Ok(ItemInteger::new(b'0', 2, day))
480477
}
481478

482479
/// %e
483480
fn fmt_day_space(&self) -> Result<ItemInteger, Error> {
484481
let day = self
485482
.tm
486-
.day
487-
.or_else(|| self.tm.to_date().ok().map(|d| d.day_ranged()))
488-
.ok_or(FE::RequiresDate)?
489-
.get();
483+
.day()
484+
.or_else(|| self.tm.to_date().ok().map(|d| d.day()))
485+
.ok_or(FE::RequiresDate)?;
490486
Ok(ItemInteger::new(b' ', 2, day))
491487
}
492488

493489
/// %I
494490
fn fmt_hour12_zero(&self) -> Result<ItemInteger, Error> {
495-
let mut hour = self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
491+
let mut hour = self.tm.hour().ok_or(FE::RequiresTime)?;
496492
if hour == 0 {
497493
hour = 12;
498494
} else if hour > 12 {
@@ -503,13 +499,13 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
503499

504500
/// %H
505501
fn fmt_hour24_zero(&self) -> Result<ItemInteger, Error> {
506-
let hour = self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
502+
let hour = self.tm.hour().ok_or(FE::RequiresTime)?;
507503
Ok(ItemInteger::new(b'0', 2, hour))
508504
}
509505

510506
/// %l
511507
fn fmt_hour12_space(&self) -> Result<ItemInteger, Error> {
512-
let mut hour = self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
508+
let mut hour = self.tm.hour().ok_or(FE::RequiresTime)?;
513509
if hour == 0 {
514510
hour = 12;
515511
} else if hour > 12 {
@@ -520,33 +516,32 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
520516

521517
/// %k
522518
fn fmt_hour24_space(&self) -> Result<ItemInteger, Error> {
523-
let hour = self.tm.hour_ranged().ok_or(FE::RequiresTime)?.get();
519+
let hour = self.tm.hour().ok_or(FE::RequiresTime)?;
524520
Ok(ItemInteger::new(b' ', 2, hour))
525521
}
526522

527523
/// %M
528524
fn fmt_minute(&self) -> Result<ItemInteger, Error> {
529-
let minute = self.tm.minute.ok_or(FE::RequiresTime)?.get();
525+
let minute = self.tm.minute().ok_or(FE::RequiresTime)?;
530526
Ok(ItemInteger::new(b'0', 2, minute))
531527
}
532528

533529
/// %m
534530
fn fmt_month(&self) -> Result<ItemInteger, Error> {
535531
let month = self
536532
.tm
537-
.month
538-
.or_else(|| self.tm.to_date().ok().map(|d| d.month_ranged()))
539-
.ok_or(FE::RequiresDate)?
540-
.get();
533+
.month()
534+
.or_else(|| self.tm.to_date().ok().map(|d| d.month()))
535+
.ok_or(FE::RequiresDate)?;
541536
Ok(ItemInteger::new(b'0', 2, month))
542537
}
543538

544539
/// %B
545540
fn fmt_month_full(&self) -> Result<ItemString, Error> {
546541
let month = self
547542
.tm
548-
.month
549-
.or_else(|| self.tm.to_date().ok().map(|d| d.month_ranged()))
543+
.month()
544+
.or_else(|| self.tm.to_date().ok().map(|d| d.month()))
550545
.ok_or(FE::RequiresDate)?;
551546
Ok(ItemString::new(Case::AsIs, month_name_full(month)))
552547
}
@@ -555,8 +550,8 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
555550
fn fmt_month_abbrev(&self) -> Result<ItemString, Error> {
556551
let month = self
557552
.tm
558-
.month
559-
.or_else(|| self.tm.to_date().ok().map(|d| d.month_ranged()))
553+
.month()
554+
.or_else(|| self.tm.to_date().ok().map(|d| d.month()))
560555
.ok_or(FE::RequiresDate)?;
561556
Ok(ItemString::new(Case::AsIs, month_name_abbrev(month)))
562557
}
@@ -611,7 +606,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
611606

612607
/// %S
613608
fn fmt_second(&self) -> Result<ItemInteger, Error> {
614-
let second = self.tm.second.ok_or(FE::RequiresTime)?.get();
609+
let second = self.tm.second().ok_or(FE::RequiresTime)?;
615610
Ok(ItemInteger::new(b'0', 2, second))
616611
}
617612

@@ -741,8 +736,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
741736
}
742737
let day = self
743738
.tm
744-
.day_of_year
745-
.map(|day| day.get())
739+
.day_of_year()
746740
.or_else(|| self.tm.to_date().ok().map(|d| d.day_of_year()))
747741
.ok_or(FE::RequiresDate)?;
748742
let weekday = self
@@ -764,9 +758,9 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
764758
fn fmt_week_iso(&self) -> Result<ItemInteger, Error> {
765759
let weeknum = self
766760
.tm
767-
.iso_week
761+
.iso_week()
768762
.or_else(|| {
769-
self.tm.to_date().ok().map(|d| d.iso_week_date().week_ranged())
763+
self.tm.to_date().ok().map(|d| d.iso_week_date().week())
770764
})
771765
.ok_or(FE::RequiresDate)?;
772766
Ok(ItemInteger::new(b'0', 2, weeknum))
@@ -780,8 +774,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
780774
}
781775
let day = self
782776
.tm
783-
.day_of_year
784-
.map(|day| day.get())
777+
.day_of_year()
785778
.or_else(|| self.tm.to_date().ok().map(|d| d.day_of_year()))
786779
.ok_or(FE::RequiresDate)?;
787780
let weekday = self
@@ -803,21 +796,19 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
803796
fn fmt_year(&self) -> Result<ItemInteger, Error> {
804797
let year = self
805798
.tm
806-
.year
807-
.or_else(|| self.tm.to_date().ok().map(|d| d.year_ranged()))
808-
.ok_or(FE::RequiresDate)?
809-
.get();
799+
.year()
800+
.or_else(|| self.tm.to_date().ok().map(|d| d.year()))
801+
.ok_or(FE::RequiresDate)?;
810802
Ok(ItemInteger::new(b'0', 4, year))
811803
}
812804

813805
/// %y
814806
fn fmt_year2(&self) -> Result<ItemInteger, Error> {
815807
let year = self
816808
.tm
817-
.year
818-
.or_else(|| self.tm.to_date().ok().map(|d| d.year_ranged()))
819-
.ok_or(FE::RequiresDate)?
820-
.get();
809+
.year()
810+
.or_else(|| self.tm.to_date().ok().map(|d| d.year()))
811+
.ok_or(FE::RequiresDate)?;
821812
let year = year % 100;
822813
Ok(ItemInteger::new(b'0', 2, year))
823814
}
@@ -826,10 +817,9 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
826817
fn fmt_century(&self) -> Result<ItemInteger, Error> {
827818
let year = self
828819
.tm
829-
.year
830-
.or_else(|| self.tm.to_date().ok().map(|d| d.year_ranged()))
831-
.ok_or(FE::RequiresDate)?
832-
.get();
820+
.year()
821+
.or_else(|| self.tm.to_date().ok().map(|d| d.year()))
822+
.ok_or(FE::RequiresDate)?;
833823
let century = year / 100;
834824
Ok(ItemInteger::new(b' ', 0, century))
835825
}
@@ -838,25 +828,23 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
838828
fn fmt_iso_week_year(&self) -> Result<ItemInteger, Error> {
839829
let year = self
840830
.tm
841-
.iso_week_year
831+
.iso_week_year()
842832
.or_else(|| {
843-
self.tm.to_date().ok().map(|d| d.iso_week_date().year_ranged())
833+
self.tm.to_date().ok().map(|d| d.iso_week_date().year())
844834
})
845-
.ok_or(FE::RequiresDate)?
846-
.get();
835+
.ok_or(FE::RequiresDate)?;
847836
Ok(ItemInteger::new(b'0', 4, year))
848837
}
849838

850839
/// %g
851840
fn fmt_iso_week_year2(&self) -> Result<ItemInteger, Error> {
852841
let year = self
853842
.tm
854-
.iso_week_year
843+
.iso_week_year()
855844
.or_else(|| {
856-
self.tm.to_date().ok().map(|d| d.iso_week_date().year_ranged())
845+
self.tm.to_date().ok().map(|d| d.iso_week_date().year())
857846
})
858-
.ok_or(FE::RequiresDate)?
859-
.get();
847+
.ok_or(FE::RequiresDate)?;
860848
let year = year % 100;
861849
Ok(ItemInteger::new(b'0', 2, year))
862850
}
@@ -865,10 +853,9 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
865853
fn fmt_quarter(&self) -> Result<ItemInteger, Error> {
866854
let month = self
867855
.tm
868-
.month
869-
.or_else(|| self.tm.to_date().ok().map(|d| d.month_ranged()))
870-
.ok_or(FE::RequiresDate)?
871-
.get();
856+
.month()
857+
.or_else(|| self.tm.to_date().ok().map(|d| d.month()))
858+
.ok_or(FE::RequiresDate)?;
872859
let quarter = match month {
873860
1..=3 => 1,
874861
4..=6 => 2,
@@ -883,8 +870,7 @@ impl<'config, 'fmt, 'tm, 'writer, 'buffer, 'data, 'write, L: Custom>
883870
fn fmt_day_of_year(&self) -> Result<ItemInteger, Error> {
884871
let day = self
885872
.tm
886-
.day_of_year
887-
.map(|day| day.get())
873+
.day_of_year()
888874
.or_else(|| self.tm.to_date().ok().map(|d| d.day_of_year()))
889875
.ok_or(FE::RequiresDate)?;
890876
Ok(ItemInteger::new(b'0', 3, day))

0 commit comments

Comments
 (0)