Bug 171241 – Unhandled enumeration values in IntlDateTimeFormat.cpp

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

[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]

[Andy VanWagoner]

Created attachment 308058 [details]
Patch

[Andy VanWagoner]

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 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]

(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]

(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]

Created attachment 308100 [details]
Patch

[JF Bastien]

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 on attachment 308100 [details]
Patch

Actually tests seem sad?

[Build Bot]

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 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]

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 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]

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 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]

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]

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]

Created attachment 308194 [details]
Patch

[JF Bastien]

Comment on attachment 308194 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 308194 [details]
Patch

Clearing flags on attachment: 308194

Committed r215792: <http://trac.webkit.org/changeset/215792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.