Bug 27902 – Missing default switch for baselineShift case statement

• Component: CSS
• Status: RESOLVED DUPLICATE

[James Hawkins]

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.

[James Hawkins]

Created attachment 33909 [details]
Assert on unhandled values for baselineShift

[Darin Adler]

Comment on attachment 33909 [details]
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.

[James Hawkins]

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?

[Alexey Proskuryakov]

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.

[Caio Marcelo de Oliveira Filho]

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