27902 2009-07-31 17:01:10 -0700 Missing default switch for baselineShift case statement 2012-05-28 21:06:50 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) All All RESOLVED DUPLICATE 83536 P2 Trivial --- 1 jhawkins webkit-unassigned cmarcelo eric oldest_to_newest 136305 0 jhawkins 2009-07-31 17:01:10 -0700 case CSSPropertyBaselineShift: { switch (svgStyle->baselineShift()) { case BS_BASELINE: return CSSPrimitiveValue::createIdentifier(CSSValueBaseline); case BS_SUPER: return CSSPrimitiveValue::createIdentifier(CSSValueSuper); case BS_SUB: return CSSPrimitiveValue::createIdentifier(CSSValueSub); case BS_LENGTH: return svgStyle->baselineShiftValue(); } } case CSSPropertyGlyphOrientationHorizontal: return glyphOrientationToCSSPrimitiveValue(svgStyle->glyphOrientationHorizontal()); If svgStyle->baselineShift() does not match any of the above four values, which could only happen if parsing was added for the value and not added here, then we'd return a value from glyphOrientationToCSSPrimitiveValue(). I basically copied the comment and assertion from a couple lines below, which checks for the same case on different level. Attaching patch shortly. 136308 1 33909 jhawkins 2009-07-31 17:03:04 -0700 Created attachment 33909 Assert on unhandled values for baselineShift 136319 2 33909 darin 2009-07-31 17:47:05 -0700 Comment on attachment 33909 Assert on unhandled values for baselineShift By adding the default case, you are turning off the gcc feature that complains if an enum value is not handled in a switch statement. So you are turning off compile-time detection (under gcc) of the very thing you are trying to catch at runtime. And compile-time detection is even better, so we don't want it turned off! Please put the assertion outside the switch statement instead of adding a default case. Or just drop this while thing, since we'll catch missing enum values immediately due to gcc's warning as soon as this file is compiled with gcc. 136718 3 jhawkins 2009-08-03 11:22:02 -0700 Ok, thanks for letting me know about that. Would you be opposed to me moving the assert outside the switch statement for the sake of older and/or non-gcc compilers? 237349 4 ap 2010-06-12 15:29:41 -0700 Yes, we can certainly add ASSERT_NOT_REACHED after the switch. That would be beneficial, because even with a compile time check, there can be e.g. memory corruption that results in a value that's not covered by case statements. 635548 5 cmarcelo 2012-05-28 21:06:50 -0700 Closing since the assertion suggested in the previous commented was added in bug 83536. The motivation was the same of this bug. *** This bug has been marked as a duplicate of bug 83536 *** 33909 2009-07-31 17:03:04 -0700 2010-06-10 17:56:28 -0700 Assert on unhandled values for baselineShift assert.diff text/plain 1899 jhawkins SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NjY0NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjEgQEAKKzIwMDktMDctMzEgIEphbWVzIEhhd2tpbnMgIDxqaGF3a2luc0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEFzc2VydCBpZiBzdmdTdHlsZS0+YmFzZWxpbmVTaGlmdCByZXR1cm5zIGFuIHVuaGFuZGxlZCB2 YWx1ZSwgYXMgdGhhdAorICAgICAgICB3b3VsZCBtZWFuIGEgdmFsdWUgd2FzIGFkZGVkIGZvciBi YXNlbGluZVNoaWZ0IHdpdGhvdXQgYmVpbmcgcHJvcGVybHkKKyAgICAgICAgaGFuZGxlZCBpbiBn ZXRTVkdQcm9wZXJ0eUNTU1ZhbHVlLCByZXN1bHRpbmcgaW4gYSBib2d1cyB2YWx1ZSBiZWluZwor ICAgICAgICByZXR1cm5lZC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTI3OTAyCisKKyAgICAgICAgTm8gdGVzdCBwb3NzaWJsZSwgYXMgdGhpcyBhc3Nl cnRpb24gY2FuIG9ubHkgYmUgcmVhY2hlZCBpZiBhIG5ldyB2YWx1ZQorICAgICAgICBpcyBhZGRl ZCBmb3IgYmFzZWxpbmVTaGlmdCB3aXRob3V0IHByb3BlciBoYW5kbGluZyBpbgorICAgICAgICBn ZXRTVkdQcm9wZXJ0eUNTU1ZhbHVlLgorCisgICAgICAgICogY3NzL1NWR0NTU0NvbXB1dGVkU3R5 bGVEZWNsYXJhdGlvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDU1NDb21wdXRlZFN0eWxlRGVj bGFyYXRpb246OmdldFNWR1Byb3BlcnR5Q1NTVmFsdWUpOiBBc3NlcnQKKyAgICAgICAgb24gdW5o YW5kbGVkIGJhc2VsaW5lU2hpZnQgdmFsdWVzLgorCiAyMDA5LTA3LTMxICBCcmFkeSBFaWRzb24g IDxiZWlkc29uQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBKb2huIFN1bGxpdmFu LgpJbmRleDogV2ViQ29yZS9jc3MvU1ZHQ1NTQ29tcHV0ZWRTdHlsZURlY2xhcmF0aW9uLmNwcAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBXZWJDb3JlL2Nzcy9TVkdDU1NDb21wdXRlZFN0eWxlRGVjbGFyYXRpb24u Y3BwCShyZXZpc2lvbiA0NjY0MSkKKysrIFdlYkNvcmUvY3NzL1NWR0NTU0NvbXB1dGVkU3R5bGVE ZWNsYXJhdGlvbi5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTE1NSw2ICsxNTUsMTAgQEAgUGFzc1Jl ZlB0cjxDU1NWYWx1ZT4gQ1NTQ29tcHV0ZWRTdHlsZURlYwogICAgICAgICAgICAgICAgICAgICBy ZXR1cm4gQ1NTUHJpbWl0aXZlVmFsdWU6OmNyZWF0ZUlkZW50aWZpZXIoQ1NTVmFsdWVTdWIpOwog ICAgICAgICAgICAgICAgIGNhc2UgQlNfTEVOR1RIOgogICAgICAgICAgICAgICAgICAgICByZXR1 cm4gc3ZnU3R5bGUtPmJhc2VsaW5lU2hpZnRWYWx1ZSgpOworICAgICAgICAgICAgICAgIGRlZmF1 bHQ6CisgICAgICAgICAgICAgICAgICAgIC8vIElmIHlvdSBjcmFzaCBoZXJlLCBpdCdzIGJlY2F1 c2UgeW91IGFkZGVkIGEgYmFzZWxpbmVTaGlmdAorICAgICAgICAgICAgICAgICAgICAvLyB2YWx1 ZSBhbmQgYXJlIG5vdCBoYW5kbGluZyBpdCBpbiB0aGlzIHN3aXRjaCBzdGF0ZW1lbnQuCisgICAg ICAgICAgICAgICAgICAgIEFTU0VSVF9XSVRIX01FU1NBR0UoMCwgInVuaW1wbGVtZW50ZWQgYmFz ZWxpbmVTaGlmdCB2YWx1ZTogJWRcbiIsIHN2Z1N0eWxlLT5iYXNlbGluZVNoaWZ0KCkpOwogICAg ICAgICAgICAgfQogICAgICAgICB9CiAgICAgICAgIGNhc2UgQ1NTUHJvcGVydHlHbHlwaE9yaWVu dGF0aW9uSG9yaXpvbnRhbDoK