Fix inverted null check in time_zone_name_win.cc#340
Fix inverted null check in time_zone_name_win.cc#340yukawa wants to merge 1 commit intogoogle:masterfrom
time_zone_name_win.cc#340Conversation
| AsProcAddress<ucal_getTimeZoneIDForWindowsID_func>( | ||
| icu_dll, "ucal_getTimeZoneIDForWindowsID"); | ||
| if (ucal_getTimeZoneIDForWindowsIDRef != nullptr) { | ||
| if (ucal_getTimeZoneIDForWindowsIDRef == nullptr) { |
There was a problem hiding this comment.
I'm fine with the change, of course, but it does seem to indicate that we don't have anything that tests whether GetWindowsLocalTimeZone() works.
There was a problem hiding this comment.
Right. The existing fallback chain in local_time_zone() makes it difficult for the test to actually fail. I've added a dedicated unit test in cbbfd28. Does it make sense?
yukawa
force-pushed
the
fix_icu_load_null_check
branch
2 times, most recently
from
e33561c to
cbbfd28
Compare
CMakeLists.txt
Outdated
| set(CCTZ_TESTS civil_time_test time_zone_format_test time_zone_lookup_test) | ||
| if(WIN32) | ||
| list(APPEND CCTZ_TESTS time_zone_name_win_test) | ||
| endif() |
There was a problem hiding this comment.
Perhaps separate list(APPEND CCTZ_TESTS ...) calls would be best factored out to after the corresponding add_test() calls? That would make each block its own separate unit, and remove the need for the extra if(WIN32) conditional.
yukawa
force-pushed
the
fix_icu_load_null_check
branch
from
cbbfd28 to
841a08a
Compare
src/time_zone_name_win_test.cc
Outdated
| HMODULE icu_dll = | ||
| ::LoadLibraryExW(L"icu.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32); | ||
| if (icu_dll != nullptr) { | ||
| const std::string tz = GetWindowsLocalTimeZone(); |
There was a problem hiding this comment.
Perhaps factor this repeated statement out of the conditional.
This commit follows up the previous commit [1], which aimed to improve the code but ended up introducing an inverted null check in time_zone_name_win.cc. As a result, LoadIcuGetTimeZoneIDForWindowsID() currently returns nullptr when it should be returning a valid time zone ID. A unit test is also added, as the fallback chain in local_time_zone() makes it difficult to verify the behavior of LoadIcuGetTimeZoneIDForWindowsID() in isolation. The test ensures that the function correctly returns a valid time zone ID on Windows. This is also a preparation for implementing TimeZoneIf with Windows time APIs (google#328). [1] 27ca173
yukawa
force-pushed
the
fix_icu_load_null_check
branch
from
841a08a to
d09a3fd
Compare
| if (icu_dll != nullptr) { | ||
| EXPECT_FALSE(tz.empty()); | ||
| ::FreeLibrary(icu_dll); | ||
| } else { | ||
| EXPECT_TRUE(tz.empty()); | ||
| } |
There was a problem hiding this comment.
My final question before handing off to @derekmauro.
Do we know which branch our Windows CI actually takes? That is, is it checking that GetWindowsLocalTimeZone() succeeds or fails?
2 participants
This commit follows up the previous commit (27ca173), which aimed to improve the code but ended up introducing an inverted null check in time_zone_name_win.cc. As a result,
LoadIcuGetTimeZoneIDForWindowsID()currently returnsnullptrwhen it should be returning a valid time zone ID.This is also a preparation for implementing TimeZoneIf with Windows time APIs (#328).