WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
171241
Unhandled enumeration values in IntlDateTimeFormat.cpp
https://bugs.webkit.org/show_bug.cgi?id=171241
Summary
Unhandled enumeration values in IntlDateTimeFormat.cpp
Michael Catanzaro
Reported
2017-04-24 12:32:23 PDT
[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]
Attachments
Patch
(2.08 KB, patch)
2017-04-24 20:50 PDT
,
Andy VanWagoner
no flags
Details
Formatted Diff
Diff
Patch
(2.34 KB, patch)
2017-04-25 07:42 PDT
,
Andy VanWagoner
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews100 for mac-elcapitan
(976.05 KB, application/zip)
2017-04-25 08:49 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
(922.76 KB, application/zip)
2017-04-25 08:52 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews112 for mac-elcapitan
(1.83 MB, application/zip)
2017-04-25 09:12 PDT
,
Build Bot
no flags
Details
Patch
(1.69 KB, patch)
2017-04-25 19:53 PDT
,
Andy VanWagoner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Andy VanWagoner
Comment 1
2017-04-24 20:50:49 PDT
Created
attachment 308058
[details]
Patch
Andy VanWagoner
Comment 2
2017-04-24 20:54:36 PDT
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?
JF Bastien
Comment 3
2017-04-24 22:04:50 PDT
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).
Andy VanWagoner
Comment 4
2017-04-25 07:29:02 PDT
(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?
Michael Catanzaro
Comment 5
2017-04-25 07:33:30 PDT
(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.
Andy VanWagoner
Comment 6
2017-04-25 07:42:39 PDT
Created
attachment 308100
[details]
Patch
JF Bastien
Comment 7
2017-04-25 08:14:23 PDT
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
JF Bastien
Comment 8
2017-04-25 08:15:04 PDT
Comment on
attachment 308100
[details]
Patch Actually tests seem sad?
Build Bot
Comment 9
2017-04-25 08:19:01 PDT
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
Build Bot
Comment 10
2017-04-25 08:49:06 PDT
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
Build Bot
Comment 11
2017-04-25 08:49:07 PDT
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
Build Bot
Comment 12
2017-04-25 08:52:18 PDT
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
Build Bot
Comment 13
2017-04-25 08:52:20 PDT
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
Build Bot
Comment 14
2017-04-25 09:12:04 PDT
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
Build Bot
Comment 15
2017-04-25 09:12:06 PDT
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
Andy VanWagoner
Comment 16
2017-04-25 19:39:10 PDT
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.
Andy VanWagoner
Comment 17
2017-04-25 19:53:52 PDT
Created
attachment 308194
[details]
Patch
JF Bastien
Comment 18
2017-04-25 21:49:50 PDT
Comment on
attachment 308194
[details]
Patch r=me
WebKit Commit Bot
Comment 19
2017-04-25 22:18:12 PDT
Comment on
attachment 308194
[details]
Patch Clearing flags on attachment: 308194 Committed
r215792
: <
http://trac.webkit.org/changeset/215792
>
WebKit Commit Bot
Comment 20
2017-04-25 22:18:14 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug