Bug 27902 - Missing default switch for baselineShift case statement
: Missing default switch for baselineShift case statement
Status: RESOLVED DUPLICATE of bug 83536
: WebKit
: 528+ (Nightly build)
: All All
: P2 Trivial
Assigned To:
  Show dependency treegraph
Reported: 2009-07-31 17:01 PST by
Modified: 2012-05-28 21:06 PST (History)

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


You need to log in before you can comment on or make changes to this bug.

Description From 2009-07-31 17:01:10 PST
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 From 2009-07-31 17:03:04 PST -------
Created an attachment (id=33909) [details]
Assert on unhandled values for baselineShift
------- Comment #2 From 2009-07-31 17:47:05 PST -------
(From update of attachment 33909 [details])
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 From 2009-08-03 11:22:02 PST -------
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 From 2010-06-12 15:29:41 PST -------
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 From 2012-05-28 21:06:50 PST -------
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 ***