[webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's

[Joe Mason]

> From: webkit-dev-bounces at lists.webkit.org 
> [webkit-dev-bounces at lists.webkit.org] on behalf of Bruno Abinader 
> [brunoabinader at gmail.com]
> Sent: Friday, August 17, 2012 12:32 PM
> To: WebKit Development
> Subject: [webkit-dev] Proposal for coding guidelines: Do not use fall-through 
> switch cases inside #ifdef's
> 
> I found a very interesting issue on some platform builds that I would
> like to share. See the example below:
> 
> ...
>     case CSSPropertyTextDecoration:
> #if ENABLE(CSS3_TEXT_DECORATION)
>     case CSSPropertyWebkitTextDecorationLine:
> #endif // CSS3_TEXT_DECORATION
>         return renderTextDecorationFlagsToCSSValue(style->textDecoration());
> ...
> 
> This breaks build on gtk, win and mac platforms as the precompiler is
> unable to understand it as a fall-through case, AFAIK. Proper
> implementation would be, then:

I find that very hard to believe, as that's a pretty huge compiler bug, and win and mac wouldn't be using the same compiler so it's unfathomable that they would share it.

What do you mean by "precompiler"?  The preprocessor?  Or is this a precompiled headers thing?

The preprocesser just does text substitution, so depending on whether CSS3_TEXT_DECORATION is enabled, this code would show up to the compiler as either:

>     case CSSPropertyTextDecoration:
>     case CSSPropertyWebkitTextDecorationLine:
>         return renderTextDecorationFlagsToCSSValue(style->textDecoration());

or

>     case CSSPropertyTextDecoration:
>         return renderTextDecorationFlagsToCSSValue(style->textDecoration());

So I don't see how the compiler can get confused about fallthrough.  The preprocessor would need to be replacing the #if block with something invalid that messes up the syntax of the case statement.

Are you sure there wasn't a typo in the "case CSSPropertyWebkitTextDecorationLine:" line which caused a perfectly normal syntax error when CSS3_TEXT_DECORATION was defined?  Or maybe CSSPropertyWebkitTextDecorationLine was not defined?

> ...
>     case CSSPropertyTextDecoration:
>         return renderTextDecorationFlagsToCSSValue(style->textDecoration());
> #if ENABLE(CSS3_TEXT_DECORATION)
>     case CSSPropertyWebkitTextDecorationLine:
>         return renderTextDecorationFlagsToCSSValue(style->textDecoration());
> #endif // CSS3_TEXT_DECORATION
> ...

I dislike this.  Repeated code should be avoided. Fallthrough is a widely used and accepted technique to do that, ever since the days of C. I can't believe there's a compiler that doesn't handle this correctly.

> Where each switch case is handled individually (if you're curious
> about it, it is fixed in bug 90493). Said this, I would like to

https://bugs.webkit.org/show_bug.cgi?id=90493 is "[chromium] Don't archive build files generated by VS2010", and doesn't seem to be related.

Joe
---------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.