| Summary: | Unhandled enumeration values in IntlDateTimeFormat.cpp | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Andy VanWagoner <andy> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | andy, bugs-noreply, buildbot, commit-queue, jfbastien, keith_miller, mark.lam, mcatanzaro, msaboff, rniwa, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Linux | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 308058 [details]
Patch
Comment on attachment 308058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308058&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965 > + default: This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added? Comment on attachment 308058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308058&action=review >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965 >> + default: > > This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added? Agreed that we probably do want to force ourselves to handle new cases instead of going to a default handler. The above comment seems to say otherwise though, but as we see from this addition that behavior would have been erroneous. That makes we wonder though: if we build with ICU 56, and then at runtime we actually use ICU 57 which can have the above two new values, will it be possible to receive a UDateFormatField field with these new values? That would lead to UB if the range of the enum's underlying type changed (say, we go from [0,255] to more values). (In reply to JF Bastien from comment #3) > Comment on attachment 308058 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308058&action=review > > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965 > >> + default: > > > > This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added? > > Agreed that we probably do want to force ourselves to handle new cases > instead of going to a default handler. The above comment seems to say > otherwise though, but as we see from this addition that behavior would have > been erroneous. > > That makes we wonder though: if we build with ICU 56, and then at runtime we > actually use ICU 57 which can have the above two new values, will it be > possible to receive a UDateFormatField field with these new values? That > would lead to UB if the range of the enum's underlying type changed (say, we > go from [0,255] to more values). We use udatpg_getBestPattern rather than building a pattern directly, so it's possible that the pattern we use for formatting ends up including one of the new field types. Especially since that should be looking at the icu data file at run time, which may be newer than the header version used at build time. We also dynamically link to libicucore, right? So the running icu functions would be aware of the new values, but our code won't. Since enums are just numbers in C++, I could put in the integer cases for those new values in a #else. While those numbers can refer to UDAT_FIELD_COUNT in older icu versions, that isn't a value we should ever actually receive. What do you think? (In reply to Andy VanWagoner from comment #4) > Since enums are just numbers in C++, I could put in the integer cases for > those new values in a #else. While those numbers can refer to > UDAT_FIELD_COUNT in older icu versions, that isn't a value we should ever > actually receive. > > What do you think? It seems like the best approach to me. Created attachment 308100 [details]
Patch
Comment on attachment 308100 [details]
Patch
Looks good. The problem I had in mind is actually kindof a non-problem now that I think about it: if the enum's underlying type change (say from uint8_t to uint16_t) then the mangled name of ICU functions which pass that enum in/out would change.
So I think what you had before was OK in that it would fall through to "literal", but adding the cases by number works too because it classifies the values properly.
r=me
Comment on attachment 308100 [details]
Patch
Actually tests seem sad?
Comment on attachment 308100 [details] Patch Attachment 308100 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3601875 New failing tests: jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-dfg-eager-no-cjit Comment on attachment 308100 [details] Patch Attachment 308100 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3601943 New failing tests: js/intl-datetimeformat.html Created attachment 308101 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308100 [details] Patch Attachment 308100 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3601945 New failing tests: js/intl-datetimeformat.html Created attachment 308102 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 308100 [details] Patch Attachment 308100 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3601962 New failing tests: js/intl-datetimeformat.html Created attachment 308104 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Looking at the test failures, it looks like a ":" between hours and minutes was miscategorized as a "dayPeriod" with the change. UDAT_TIME_SEPARATOR_FIELD = 35 makes trying to use the raw numbers incorrect. I'm going to go back to just including the new values in an #if and remove the #else with numbers. Created attachment 308194 [details]
Patch
Comment on attachment 308194 [details]
Patch
r=me
Comment on attachment 308194 [details] Patch Clearing flags on attachment: 308194 Committed r215792: <http://trac.webkit.org/changeset/215792> All reviewed patches have been landed. Closing bug. |
[925/5514] Building CXX object Source/...e.dir/runtime/IntlDateTimeFormat.cpp.o ../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp: In static member function ‘static const char* JSC::IntlDateTimeFormat::partTypeString(UDateFormatField)’: ../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:909:12: warning: enumeration value ‘UDAT_AM_PM_MIDNIGHT_NOON_FIELD’ not handled in switch [-Wswitch] switch (field) { ^ ../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:909:12: warning: enumeration value ‘UDAT_FLEXIBLE_DAY_PERIOD_FIELD’ not handled in switch [-Wswitch]