make FixedTimeZones be isbits by using ShortStrings #327
Conversation
|
The CI/Benchmarks job is failing because it is trying to deserialize a something that has had it's type represenation changed. |
Tweak comment tweak comment do use split with short strings Julia needs brakets for function application
| @@ -72,7 +76,7 @@ UTC+15:45:21 | |||
| function FixedTimeZone(s::AbstractString) | |||
| s == "Z" && return UTC_ZERO | |||
|
|
|||
| m = match(FIXED_TIME_ZONE_REGEX, s) | |||
| m = match(FIXED_TIME_ZONE_REGEX, String(s)) | |||
There was a problem hiding this comment.
Did you see an error before changing this? I'll note SubString{String} also works here so we don't always need to call String
There was a problem hiding this comment.
Yes, it errors before
Iirc I looked in the docs for regex and it explicitly said it only worked on String. Which is a weird thing for Julia docs to say.
And also weird that it doesn't convert abstract strings to strings for it.
| @@ -1,3 +1,7 @@ | |||
| # Ideally would always use ShortString15, but it's `hash` is broken on 32-bit systems. | |||
| # https://github.com/JuliaString/MurmurHash3.jl/issues/12 | |||
| const FixedTimeZoneName = Int === Int64 ? ShortString15 : String | |||
There was a problem hiding this comment.
I really wish the 32-bit problem was properly fixed... if this is what we need to do then let's be as clear about it as we can.
| const FixedTimeZoneName = Int === Int64 ? ShortString15 : String | |
| const FixedTimeZoneName = Sys.WORD_SIZE == 64 ? ShortString15 : String |
There was a problem hiding this comment.
It is now fixed. So this PR needs updating
| @@ -30,7 +34,7 @@ const FIXED_TIME_ZONE_REGEX = r""" | |||
| A `TimeZone` with a constant offset for all of time. | |||
| """ | |||
| struct FixedTimeZone <: TimeZone | |||
| name::String | |||
| name::FixedTimeZoneName | |||
There was a problem hiding this comment.
Once ShortStrings.jl is fixed this type alias won't survive. May as well try to indicate what it truly is.
| name::FixedTimeZoneName | |
| name::_ShortString15 |
|
Closed by #354 |
Contributes towards #271 ,
following up on just this part of #324 (comment)
requires JuliaString/ShortStrings.jl#46
Is less extreme than #326