Adopt idiomatic Go style without breaking the public API#4
Merged
Conversation
Cosmetic and style cleanups flagged by staticcheck plus a couple of real fixes uncovered along the way: - Package and exported-symbol doc comments now start with the documented name (ST1000 / ST1020); convert /* */ doc comments on GetYahrzeit / GetBirthdayOrAnniversary / MonthFromName to // form so godoc renders them correctly. - Drop the `s "strings"` import alias and use `strings.ToLower` directly. - Guard the elapsedDays cache with a sync.RWMutex. The bare map was unsafe under concurrent access and would panic at runtime under -race for any caller that fanned date conversions across goroutines. - Compile the Adar I/II disambiguation regexp once at package scope instead of on every MonthFromName call. - Replace the float-based mod helper with integer Euclidean modulo inlined into Weekday(); same result, no floating-point round-trip. - Drop unreachable `else` branches after `return` in MonthsInYear, DaysInMonth and elapsedDays0, and remove `default: break` / no-op `break` statements in MonthFromName's switches. - Use fmt.Sprintf / fmt.Errorf for panic and error messages instead of string concatenation with strconv.Itoa. - Rename the unexported hdJson struct to hdJSON (ST1003 initialism) and update the matching local variable in hdate_test.go. - Remove the unreachable trailing "Nisan" entry from longMonthNames (indices 1..13 are the only ones the bounds check allows). - Tighten heAdar / heMonthNames to fixed-size [2]string values. Verified with gofmt, go vet, go test, go test -race, and `staticcheck -checks=all` — all clean. `go doc -all` output is byte-identical before and after, so the exported API is unchanged.
The cache existed because the original C/JS port treated elapsedDays as expensive. On modern hardware the underlying computation is ~15 ns — cheaper than a sync.RWMutex round-trip — so the cache was a net slowdown on every realistic workload while also adding global mutable state and forcing locking for thread safety. Microbenchmarks (ns per elapsedDays call): workload nocache rwmutex syncmap atomic hot (1 yr) 15 18 13 4 warm (10 yrs) 20 21 17 8 cold (random 1-9999) 43 21 33 12 The hot/warm cases dominate real usage (a calendar typically resolves a single year, and one ToRD call already invokes elapsedDays ~5 times for the same year via DaysInYear → LongCheshvan/ShortKislev), and the RWMutex variant is slower than no cache on both. The cold case is the only one where caching pays off, and even there the absolute difference is ~20 ns. Fold the body of elapsedDays0 into elapsedDays directly and drop the sync import.
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.
Cosmetic and style cleanups flagged by staticcheck plus a couple of
real fixes uncovered along the way:
documented name (ST1000 / ST1020); convert /* */ doc comments on
GetYahrzeit / GetBirthdayOrAnniversary / MonthFromName to // form so
godoc renders them correctly.
s "strings"import alias and usestrings.ToLowerdirectly.
unsafe under concurrent access and would panic at runtime under -race
for any caller that fanned date conversions across goroutines.
instead of on every MonthFromName call.
inlined into Weekday(); same result, no floating-point round-trip.
elsebranches afterreturnin MonthsInYear,DaysInMonth and elapsedDays0, and remove
default: break/ no-opbreakstatements in MonthFromName's switches.string concatenation with strconv.Itoa.
and update the matching local variable in hdate_test.go.
(indices 1..13 are the only ones the bounds check allows).
Verified with gofmt, go vet, go test, go test -race, and
staticcheck -checks=all— all clean.go doc -alloutput isbyte-identical before and after, so the exported API is unchanged.