Modernize Go style and make the calendar math table-driven#7
Merged
Conversation
The shape of a Hebrew year is fully determined by its length, and there
are only six possible lengths. Tabulate the month lengths and the day
offset of each month within the year once at startup, so the conversions
become lookups instead of loops.
Previously ToRD summed month lengths one at a time, and each DaysInMonth
in that loop could call DaysInYear, which computes elapsedDays twice. Two
nested loops made FromRD worse: it searched for the month by calling ToRD
once per candidate, so the whole month-summing loop ran again on every
step of the search.
Also make HDate an immutable value. Abs() had a pointer receiver and
memoized into hd.abs, but every other method has a value receiver, so
Weekday(), Next() and friends took the address of a temporary copy and
threw the memo away -- New() never set abs, so those paths recomputed
ToRD on every single call. Compute abs at construction instead and give
Abs() a value receiver. Widening a method from a pointer to a value
receiver only adds to the method set, so this does not break consumers.
ToRD 43.4 ns -> 5.8 ns
FromRD 155 ns -> 13.4 ns
RoundTrip 156 ns -> 13.4 ns
Greg 1125 ns -> 13.6 ns
FromGregorian 384 ns -> 18.5 ns
Next 61.9 ns -> 11.9 ns
Weekday 6.6 ns -> 1.6 ns
DaysInMonth 8.1 ns -> 5.6 ns
GetYahrzeit 96.2 ns -> 47.2 ns
MonthFromName 44.2 ns -> 3.1 ns 1 alloc -> 0
MonthName("he") 8.7 ns -> 4.7 ns
String 57.5 ns -> 35.5 ns 2 allocs -> 1
StripNikkud 95.5 ns -> 60.4 ns
StripNikkud/clean 83.6 ns -> 16.9 ns 2 allocs -> 0
Greg() owes most of its win to the greg v1.1.0 bump. New() regresses
3.8 ns -> 7.4 ns because it now computes abs up front, which is what
buys Weekday, Next and Greg their speedups; anything that actually uses
an HDate comes out far ahead.
Fix DayOnOrBefore for dates before the common era. Go's % truncates
toward zero, so for the negative R.D. numbers of Hebrew years before
~3762 it returned a day *after* rataDie, which is the opposite of what
the function promises. It placed Shabbat Shuva after Yom Kippur and
Shabbat Zachor after Purim -- every special Shabbat landed exactly a
week late. Floor the modulus by hand. This is the only behavior change
in this commit; hebcal-go's golden event counts for Hebrew years 1 and 2
encode the old bug and need updating.
Tests: a differential test pinning the tables against the original
loop-based implementations over every month and day of Hebrew years
1-8000, including the out-of-range months the old loops accepted; an
exhaustive round trip over all ~2.9M days from the epoch forward; and a
guard that DayOnOrBefore lands on the requested weekday for negative R.D.
Also:
- UnmarshalJSON reported no error for out-of-range values, silently
producing an HDate that no calendar day corresponds to. Validate via a
non-panicking constructor and return the error.
- MonthName called strings.ToLower on every invocation, allocating just
to compare against two constants; use EqualFold. Index the Hebrew month
names with an array rather than hashing a map.
- MonthFromName allocated a lowercased copy and a []rune of the whole
string, then read two runes of it. Decode just those two.
- Export ErrMonthName so callers can use errors.Is instead of matching
on the message.
- HebrewStripNikkud converted to []rune to iterate (range already yields
runes) and sized Grow in runes while writing bytes. Return the string
unchanged when there is nothing to strip.
- GetYahrzeit assigned to another HDate's private month/day fields,
leaving a stale abs behind. Use local variables.
- go.mod: go 1.17 -> 1.18, greg v1.0.2 -> v1.1.0. CI pinned Go 1.17 and
never ran vet; test on a [1.18, stable] matrix and vet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps Go to 1.18 and
gregto v1.1.0, modernizes the package for current Go style, and speeds up the conversion hot paths. The exported API is unchanged, with one deliberate behavior fix called out below.Table-driven calendar math
A Hebrew year's entire shape — leap or not, long Cheshvan, short Kislev — is determined by its length, and there are only six possible lengths. The month lengths and each month's day-offset within the year are now tabulated once at startup, so conversions are lookups instead of loops.
ToRDsummed month lengths one at a time, and eachDaysInMonthin that loop could callDaysInYear, which computeselapsedDaystwice.FromRDwas worse: it searched for the month by callingToRDonce per candidate, so the whole month-summing loop ran again on every step of the search.HDateis now an immutable valueAbs()had a pointer receiver and memoized intohd.abs, but every other method has a value receiver — soWeekday(),Next()and friends took the address of a temporary copy and threw the memo away.New()never setabs, so those paths recomputedToRDon every single call.absis now computed at construction andAbs()has a value receiver.Widening a method from a pointer to a value receiver only adds to the method set, so this does not break consumers.
Benchmarks
Greg()owes most of its win to the greg v1.1.0 bump.New()regresses 3.8 ns -> 7.4 ns because it now computesabsup front, which is what buysWeekday,NextandGregtheir speedups; anything that actually uses anHDatecomes out far ahead.DayOnOrBeforebefore the common eraGo's
%truncates toward zero, so for the negative R.D. numbers of Hebrew years before ~3762,DayOnOrBeforereturned a day afterrataDie— the opposite of what it promises. Downstream this placed Shabbat Shuva after Yom Kippur and Shabbat Zachor after Purim; every special Shabbat landed exactly a week late. The modulus is now floored by hand.This is the only behavior change in the PR. Verified by building hebcal-go against this branch: with just this one fix reverted, all of hebcal-go passes, so nothing else in the rewrite moves any output. Modern dates are unaffected.
This requires a follow-up in hebcal-go. Its golden event counts for Hebrew years 1 and 2 encode the old bug and must be updated together with a bump to the hdate release containing this fix:
TestYear1: 79 -> 80TestYear2: 127 -> 128Suggest tagging this as v1.4.0 (minor, not patch) on account of the behavior change.
Testing
Because the calendar core was rewritten, passing the existing tests wasn't sufficient. Added:
FromRD/ToRD/Newall agree;DayOnOrBeforelands on the requested weekday for negative R.D.go vetclean,gofmtclean,go test -raceclean.Also
UnmarshalJSONreported no error for out-of-range values, silently producing anHDatethat no calendar day corresponds to (e.g.{"hd": 99}). It now validates via a non-panicking constructor and returns an error. This is stricter than before — anything feeding hdate sloppy JSON will now see errors where it previously got a silently-corrupt date.MonthNamecalledstrings.ToLoweron every invocation, allocating just to compare against two constants; now usesEqualFold. Hebrew month names moved from a map to an array.MonthFromNameallocated a lowercased copy and a[]runeof the whole string, then read two runes of it. Now decodes just those two.ErrMonthNameso callers can useerrors.Isinstead of matching on the message.HebrewStripNikkudconverted to[]runeto iterate (rangealready yields runes) and sizedGrowin runes while writing bytes. Now returns the string unchanged when there is nothing to strip.GetYahrzeitassigned to anotherHDate's privatemonth/dayfields, leaving a staleabsbehind. Now uses local variables.vet, so modern checks had never run against this code. Now a[1.18, stable]matrix with vet.🤖 Generated with Claude Code