Introduce a Shortstring based Name type#324
Introduce a Shortstring based Name type#324oxinabox wants to merge 11 commits intoJuliaTime:masterfrom
Conversation
| @test ZonedDateTime(local_dt, warsaw, true).zone.name == "CET" | ||
| @test ZonedDateTime(local_dt, warsaw, false).zone.name == "CET" | ||
| @test ZonedDateTime(utc_dt, warsaw, from_utc=true).zone.name == "CET" | ||
| @test string(ZonedDateTime(local_dt, warsaw).zone.name) == "CET" |
There was a problem hiding this comment.
Implementing promotion rules should avoid all these changes
There was a problem hiding this comment.
I don't think so. Happy to hear if i am wrong though.
Subtyping AbstractString would (but that is more work see #324 (comment))
overloading equality would (but that can cause probelms since it would also require making hash agree and that is expensive to do)
AFIACT there is no promotion rule that gets invoked for == between Strign and a random type.
| # """ | ||
|
|
||
| struct ZonedDateTime <: AbstractDateTime | ||
| struct ZonedDateTime{T<:TimeZone} <: AbstractDateTime |
There was a problem hiding this comment.
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.
It doesn't achieve the goal of making a ZonedDateTime with a FixedTimeZone isbits without this.
There was a problem hiding this comment.
But i could remove it from this PR.
though we would lose many of the benifits.
(we would get most of those benifits from just #327)
though it would make it easier to make a PR that pushed the timezone as a value in the type-parameter.
Which might be the preferred breaking change.
| region::ShortString15 | ||
| locality1::ShortString15 | ||
| locality2::ShortString15 | ||
| end |
There was a problem hiding this comment.
Maybe should be a subtype of AbstractString?
There was a problem hiding this comment.
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 SNames
There was a problem hiding this comment.
Why not just use a larger ShortString and skip this custom type all together? If ShortString63 is larger than you want you could always tweak the size by defining a primitive type and use ShortString{T}. This gets you the string functionality without you having to write it.
There was a problem hiding this comment.
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
julia> @btime x==y setup=(x=convert(TimeZones.SName, "America/Winnipeg"); y=convert(TimeZones.SName, "America/Argentina/ComodRivadavia"))
1.504 ns (0 allocations: 0 bytes)
false
julia> @btime x==y setup=(x=ShortString63("America/Winnipeg"); y=ShortString63("America/Argentina/ComodRivadavia"))
2.117 ns (0 allocations: 0 bytes)
false== true
(basically exactly the same as the false case)
julia> @btime x==y setup=(x=convert(TimeZones.SName, "America/Winnipeg"); y=convert(TimeZones.SName, "America/Winnipeg"))
1.505 ns (0 allocations: 0 bytes)
true
julia> @btime x==y setup=(x=ShortString63("America/Winnipeg"); y=ShortString63("America/Winnipeg"))
2.115 ns (0 allocations: 0 bytes)
truehash
julia> @btime hash(x)==hash(y) setup=(x=convert(TimeZones.SName, "America/Winnipeg"); y=convert(TimeZones.SName, "America/Argentina/ComodRivadavia"))
34.256 ns (0 allocations: 0 bytes);
julia> @btime hash(x)==hash(y) setup=(x=ShortString63("America/Winnipeg"); y=ShortString63("America/Argentina/ComodRivadavia"));
27.598 ns (0 allocations: 0 bytes)So I will make that change, since it is simpler
|
Tests pass right now. @omus managed to make Intervals deserialize happily across similar changes, so maybe that can be done here also? |
2 participants
A possible partial resolution to #271
Doesn't solve the
VariableTimeZone's having aVectoras afieldAt the moment tests are failing. That probably is not significiant. just some last things to chase up e.g. with show overloads