[android] Support for time zones in Android. #2168

drodriguez · 2019-04-24T01:06:36Z

The time zone database in Android sits in a non-standard place, and it
can be overriden by a second location. Besides not sitting in a standard
place, it differs from other Unixes in that the database is bundled into
one binary file, and there are no folders with small files for each time
zone. Luckily the database is generated from Olson, so most of the
CFTimeZone code already handles the details.

The code implements a generic enumeration of the time zones contained in
the database (checking for both the override and the standard location).
The enumeration code is used for both enumerating the known time zones,
as well as creating time zones by name.

At many points there was code blocking the usage of time zones in
Android that has been removed.

This allows the tests in TestDateFormatter, TestCalendar and
TestTimeZone to pass on Android.

CoreFoundation/NumberDate.subproj/CFTimeZone.c

spevans · 2019-04-24T07:19:53Z

@swift-ci test

CoreFoundation/NumberDate.subproj/CFTimeZone.c

drodriguez

Thanks for the feedback. I will split the PR in two, and fix all the memory management problems.

CoreFoundation/NumberDate.subproj/CFTimeZone.c

drodriguez · 2019-04-24T17:48:07Z

This might not apply cleanly without #2172. But one can look only at the second commit, if you want the Android specific changes only.

drodriguez · 2019-04-24T19:05:49Z

After fighting a little with the removal of __InitTZStrings and the multiple conditional compilation branches it introduces, I don’t think it is a great idea, but that's the code that it is up for review. If we want to back out of that change because it introduces complexity, we are still on time to do it.

CoreFoundation/NumberDate.subproj/CFTimeZone.c

compnerd · 2019-04-25T17:29:56Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

@@ -702,7 +829,9 @@ static CFTimeZoneRef __CFTimeZoneCreateSystem(void) {
        } else {
            name = CFStringCreateWithBytes(kCFAllocatorSystemDefault, (uint8_t *)linkbuf, strlen(linkbuf), kCFStringEncodingUTF8, false);
        }
-    } else {
+    } else


I think it might be nicer to just end the previous block with the one duplicated line for the return and then avoid the else and braces for the remainder.

I think it is not possible here, since there’s a block in the middle that has to be run for both branches. The return cannot be moved here.

Hmm, did I miss that? What is the block which needs to be executed?

The if (name) block that starts in L890 (new version). The previous branches (the if and the else branch, only the else is valid for Android), are trying to find the name, while that last bit is find the information about the time zone with that name.

How about folding !TARGET_OS_ANDROID into the if condition on L868?

I'm not sure exactly why you mean (please provide the code of what you mean), but __tzZoneInfo is used in L866, and that variable doesn't exist in Android, so it has to be compiled out.

CoreFoundation/NumberDate.subproj/CFTimeZone.c

drodriguez · 2019-05-01T22:42:30Z

@swift-ci please test

drodriguez · 2019-05-02T21:06:53Z

@compnerd, @spevans: thanks for all the feedback already. I was wondering if there's more pieces that anyone thinks need improving, or we can plan to merge this into master. Thanks!

The time zone database in Android sits in a non-standard place, and it can be overridedn by a second location. Besides not sitting in a standard place, it differs from other Unixes in that the database is bundled into one binary file, and there are no folders with small files for each time zone. Luckily the database is generated from Olson, so most of the CFTimeZone code already handles the details. The code implements a generic enumeration of the time zones contained in the database (checking for both the override and the standard location). The enumeration code is used for both enumerating the known time zones, as well as creating time zones by name. At many points there was code blocking the usage of time zones in Android that has been removed. This allows the tests in TestDateFormatter, TestCalendar and TestTimeZone to pass on Android.

drodriguez · 2019-05-13T21:37:18Z

Fixed some merge errors for the Android code.

@swift-ci please test

finagolfin · 2019-05-14T16:12:01Z

Heh, I had to make almost the same changes to the previous commit to get it to compile, though I didn't spot the fclose fix and it would segfault.

Now, several dozen more timezone tests pass on Android AArch64 with this latest commit, nice work.

compnerd

Minor style things which I think would be nice, but I think this is good to go otherwise.

compnerd · 2019-05-15T16:05:38Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

+ * - context1: The argument passed into __CFAndroidTimeZoneListEnumerate.
+ * - context2: The argument passed into __CFAndroidTimeZoneListEnumerate.
+ */
+typedef Boolean (*__CFAndroidTimeZoneListEnumerateCallback)(const char name[ANDROID_TZ_ENTRY_NAME_LENGTH], int32_t offset, int32_t length, FILE *fp, void *context1, void *context2);


I would give them proper names rather than context1 and context2. (e.g. name, data)

They are name and data in the case you are calling __CFTimeZoneDataCreateCallback, but context1 is actually a CFMutableArrayRef in the case of calling __CFCopyAndroidTimeZoneListCallback. That's why I used just one context before.

compnerd · 2019-05-15T16:11:32Z

CoreFoundation/NumberDate.subproj/CFTimeZone.c

@@ -702,7 +829,9 @@ static CFTimeZoneRef __CFTimeZoneCreateSystem(void) {
        } else {
            name = CFStringCreateWithBytes(kCFAllocatorSystemDefault, (uint8_t *)linkbuf, strlen(linkbuf), kCFStringEncodingUTF8, false);
        }
-    } else {
+    } else


How about folding !TARGET_OS_ANDROID into the if condition on L868?

CoreFoundation/NumberDate.subproj/CFTimeZone.c

Foundation/NSTimeZone.swift

Revert an Android-specific block that was already removed in swiftlang#2168 and don't check for baseURL in _CFTimeZoneInit(), since Android doesn't use it anyway.

drodriguez requested a review from compnerd April 24, 2019 01:06

drodriguez force-pushed the android-time-zone-database branch from e416b11 to 76bd79d Compare April 24, 2019 01:52

spevans reviewed Apr 24, 2019

View reviewed changes

CoreFoundation/NumberDate.subproj/CFTimeZone.c Show resolved Hide resolved

compnerd requested changes Apr 24, 2019

View reviewed changes

drodriguez commented Apr 24, 2019

View reviewed changes

drodriguez mentioned this pull request Apr 24, 2019

Move duplicated code into its own function. #2172

Merged

drodriguez force-pushed the android-time-zone-database branch from 76bd79d to cf58b49 Compare April 24, 2019 17:46

drodriguez force-pushed the android-time-zone-database branch from cf58b49 to 473cd46 Compare April 24, 2019 19:01

compnerd reviewed Apr 25, 2019

View reviewed changes

drodriguez force-pushed the android-time-zone-database branch from 473cd46 to 102da92 Compare April 26, 2019 00:08

drodriguez force-pushed the android-time-zone-database branch from 102da92 to 6454141 Compare May 13, 2019 21:35

compnerd approved these changes May 15, 2019

View reviewed changes

drodriguez merged commit 7e67c02 into swiftlang:master May 16, 2019

drodriguez deleted the android-time-zone-database branch May 16, 2019 19:46

finagolfin mentioned this pull request Oct 11, 2021

[CoreFoundation] Revert some Android regressions that crept in #3088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Support for time zones in Android. #2168

[android] Support for time zones in Android. #2168

drodriguez commented Apr 24, 2019 •

edited

Loading

spevans commented Apr 24, 2019

drodriguez left a comment

drodriguez commented Apr 24, 2019

drodriguez commented Apr 24, 2019

compnerd Apr 25, 2019

drodriguez Apr 25, 2019

compnerd Apr 27, 2019

drodriguez Apr 28, 2019

compnerd May 15, 2019

drodriguez May 15, 2019

drodriguez commented May 1, 2019

drodriguez commented May 2, 2019

drodriguez commented May 13, 2019

finagolfin commented May 14, 2019

compnerd left a comment

compnerd May 15, 2019

drodriguez May 15, 2019

compnerd May 15, 2019

[android] Support for time zones in Android. #2168

[android] Support for time zones in Android. #2168

Conversation

drodriguez commented Apr 24, 2019 • edited Loading

spevans commented Apr 24, 2019

drodriguez left a comment

Choose a reason for hiding this comment

drodriguez commented Apr 24, 2019

drodriguez commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodriguez commented May 1, 2019

drodriguez commented May 2, 2019

drodriguez commented May 13, 2019

finagolfin commented May 14, 2019

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodriguez commented Apr 24, 2019 •

edited

Loading