Bug 27902 - Missing default switch for baselineShift case statement
Summary: Missing default switch for baselineShift case statement
Status: RESOLVED DUPLICATE of bug 83536
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-31 17:01 PDT by James Hawkins
Modified: 2012-05-28 21:06 PDT (History)
2 users (show)

See Also:


Attachments
Assert on unhandled values for baselineShift (1.85 KB, patch)
2009-07-31 17:03 PDT, James Hawkins
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Hawkins 2009-07-31 17:01:10 PDT
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.
Comment 1 James Hawkins 2009-07-31 17:03:04 PDT
Created attachment 33909 [details]
Assert on unhandled values for baselineShift
Comment 2 Darin Adler 2009-07-31 17:47:05 PDT
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.
Comment 3 James Hawkins 2009-08-03 11:22:02 PDT
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?
Comment 4 Alexey Proskuryakov 2010-06-12 15:29:41 PDT
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.
Comment 5 Caio Marcelo de Oliveira Filho 2012-05-28 21:06:50 PDT
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 ***