-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a Shortstring based Name type #324
Changes from all commits
9125776
f07d246
d5dd9bc
b12c7db
9e7b073
1c4527a
2ec09bf
49bbd30
e3d8d0d
faf0a29
68a4fb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
struct SName | ||
region::ShortString15 | ||
locality1::ShortString15 | ||
locality2::ShortString15 | ||
end | ||
|
||
function Base.print(io::IO, name::SName) | ||
print(io, name.region) | ||
if !isempty(name.locality1) | ||
print(io,"/", name.locality1) | ||
if !isempty(name.locality2) | ||
print(io,"/", name.locality2) | ||
end | ||
end | ||
end | ||
|
||
Base.convert(::Type{String}, name::SName) = string(name) | ||
function Base.convert(::Type{SName}, str::AbstractString) | ||
name = try_convert(SName, str) | ||
if name isa Nothing | ||
throw(DomainError(str, "Timezone must have 3 or fewer parts, all with length < 16")) | ||
end | ||
return name | ||
end | ||
|
||
try_convert(::Type{SName}, name::SName) = name | ||
try_convert(::Type{String}, name::String) = name | ||
function try_convert(::Type{SName}, str::AbstractString) | ||
parts = split(str, "/") | ||
(length(parts) <= 3) || return nothing | ||
all(length(part)<16 for part in parts) || return nothing | ||
return if length(parts) == 3 | ||
SName(parts[1], parts[2], parts[3]) | ||
elseif length(parts) == 2 | ||
SName(parts[1], parts[2], ss15"") | ||
else | ||
SName(parts[1], ss15"", ss15"") | ||
end | ||
omus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
|
||
Base.isempty(name::SName) = isempty(name.region) # region being empty implies all empty | ||
|
||
name_parts(str::AbstractString) = split(str, "/") | ||
function name_parts(name::SName) | ||
# TODO this could be faster by returning an iterator but not really performance critial | ||
parts = [name.region] | ||
if !isempty(name.locality1) | ||
push!(parts, name.locality1) | ||
if !isempty(name.locality2) | ||
push!(parts, name.locality2) | ||
end | ||
end | ||
return parts | ||
end | ||
|
||
# Short strings are broken on 32bit: | ||
# TODO: https://github.com/JuliaString/MurmurHash3.jl/issues/12 | ||
const Name = Int === Int32 ? String : SName |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,22 +6,20 @@ using Dates: AbstractDateTime, argerror, validargs | |
# A `DateTime` that includes `TimeZone` information. | ||
# """ | ||
|
||
struct ZonedDateTime <: AbstractDateTime | ||
struct ZonedDateTime{T<:TimeZone} <: AbstractDateTime | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I learned from Intervals.jl changing the type parameters like this should be considered a breaking change. I'd probably punt this as part of this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't achieve the goal of making a ZonedDateTime with a FixedTimeZone isbits without this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i could remove it from this PR. though it would make it easier to make a PR that pushed the timezone as a value in the type-parameter. |
||
utc_datetime::DateTime | ||
timezone::TimeZone | ||
timezone::T | ||
zone::FixedTimeZone # The current zone for the utc_datetime. | ||
end | ||
|
||
function ZonedDateTime(utc_datetime::DateTime, timezone::TimeZone, zone::FixedTimeZone) | ||
return new(utc_datetime, timezone, zone) | ||
function ZonedDateTime( | ||
utc_datetime::DateTime, timezone::VariableTimeZone, zone::FixedTimeZone | ||
) | ||
if timezone.cutoff !== nothing && utc_datetime >= timezone.cutoff | ||
throw(UnhandledTimeError(timezone)) | ||
end | ||
|
||
function ZonedDateTime(utc_datetime::DateTime, timezone::VariableTimeZone, zone::FixedTimeZone) | ||
if timezone.cutoff !== nothing && utc_datetime >= timezone.cutoff | ||
throw(UnhandledTimeError(timezone)) | ||
end | ||
|
||
return new(utc_datetime, timezone, zone) | ||
end | ||
return ZonedDateTime{VariableTimeZone}(utc_datetime, timezone, zone) | ||
end | ||
|
||
""" | ||
|
@@ -181,11 +179,11 @@ function Base.hash(zdt::ZonedDateTime, h::UInt) | |
return h | ||
end | ||
|
||
Base.typemin(::Type{ZonedDateTime}) = ZonedDateTime(typemin(DateTime), utc_tz; from_utc=true) | ||
Base.typemax(::Type{ZonedDateTime}) = ZonedDateTime(typemax(DateTime), utc_tz; from_utc=true) | ||
Base.typemin(::Type{<:ZonedDateTime}) = ZonedDateTime(typemin(DateTime), utc_tz; from_utc=true) | ||
Base.typemax(::Type{<:ZonedDateTime}) = ZonedDateTime(typemax(DateTime), utc_tz; from_utc=true) | ||
|
||
# Note: The `validargs` function is as part of the Dates parsing interface. | ||
function Dates.validargs(::Type{ZonedDateTime}, y::Int64, m::Union{Int64, Int32}, d::Int64, h::Int64, mi::Int64, s::Int64, ms::Int64, tz::AbstractString) | ||
function Dates.validargs(::Type{<:ZonedDateTime}, y::Int64, m::Union{Int64, Int32}, d::Int64, h::Int64, mi::Int64, s::Int64, ms::Int64, tz::AbstractString) | ||
err = validargs(DateTime, y, Int64(m), d, h, mi, s, ms) | ||
err === nothing || return err | ||
istimezone(tz) || return argerror("TimeZone: \"$tz\" is not a recognized time zone") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should be a subtype of
AbstractString
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering it.
But it is a lot of work to subtype AbstractString. Especially because we need to match equality and thus hash with
String
.And this is a internal type that is used for an internal field.
And the main operation that matters is comparing for equality with other
SName
sThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use a larger
ShortString
and skip this custom type all together? IfShortString63
is larger than you want you could always tweak the size by defining a primitive type and useShortString{T}
. This gets you the string functionality without you having to write it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think that is viable than sure.
Based on your earlier comments that i reinterated here #271 (comment)
I thought it wasn't
In particular due to the BitInterger seralization one (which i now realized has been fixed).
I also though that they would be slower, as i have found that they are much slower than expected once you get large, but it seems that that doesn't really kick in for ShortString63.
It is a bit slower for some operations but a bit faster for others.
Benchmarks
== false
== true
(basically exactly the same as the false case)
hash
So I will make that change, since it is simpler