search

pavkam / tzdb

Delphi/FPC Time Zone Database
https://www.iana.org/time-zones
BSD 3-Clause "New" or "Revised" License
84 stars 27 forks source link

Replace Contnrs with Generics.Dictionary #6

Closed ccy closed 6 years ago

pavkam commented 6 years ago

Thanks @ccy, I'll take a look.

mbtaylor1982 commented 6 years ago

With out doing too much work I don't think we can simply change TBucketList for TDictionary as it would prevent using the TZDB in pre-generics versions of Delphi and FPC. Maybe using a TStringList to store the year as a string and in the corresponding object property store a pointer to the list of rules for the year would work?

I'm not sure if this is the best approach but I'm reasonably sure it will be supported in both older and newer versions of Delphi also FPC and on mobile targets too.

ccy commented 6 years ago

I didn't study your code in detail. I am not too sure the reason you use hashing list like TBucketList. By using hashing list, I assume there are huge number key-value pairs stored (e.g.: more than 10000) to gain performance for accessing.

If the list won't grow to the size, using TStringList is a wise choice too.

pavkam commented 6 years ago

@ccy A hash-based dictionary in normal cases has better performance for more than 10 items. It is better to use a dictionary in most cases in which indexing is by non-integer key.

I chose those classes exactly because I wanted compatibility with Delphi 7 (I think!) and FPC 2.2.

Also I wanted to avoid the use of Generics in Delphi XE at all costs, due to the "bugginess" of the compiler.

I suggest the easiest way, in order to ensure compatibility, is to IFDEF to TDictionary for whatever is the newest version of the compiler that supports iOS/Android. everything else would use the old containers.

Alex.

mbtaylor1982 commented 6 years ago

@pavkam looking at it maybe an $IFDEF is the best approach to maintain performance, I was looking at possibly using THashedStringList to replace TBucketList but I'm not 100% sure it was available in Delphi 7, also I fear the int -> str and str -> int conversion involved would impact performance.

The reason I didn't initially say use an $IFDEF is it makes testing difficult as you need multiple development environments setup, because of this I was looking for a fix using the lowest common denominator.

pavkam commented 6 years ago

@mbtaylor1982, yeah, that's understandable. IFDEFs are evil. That being said, you are correct we have to IFDEF for newest version to use TDictionary.

pavkam commented 6 years ago

I will merge this PR. I think there is no merit in keeping old versions of Delphi alive. As long as FPC works I am OK.