Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On Aug 21, 2012, at 1:22 PM, Mark Rowe wrote: > It'd be preferable to fix whatever problem exists with the dependencies so > that these files are automatically regenerated when needed. Otherwise anyone > building on these platforms, including buildbots, will just run in to the > issue after you land the patch. I agree. And if we can find a reproducible case of the build dependencies not working, that should make it easier to diagnose and fix. If we can’t fix the problem, it’s still preferable to check something in that triggers a full build rather than telling the bot to do so, since that will probably fix the problem for other people too, not just the bots. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On 2012-08-21, at 13:02, Bruno Abinader wrote: > On Sat, Aug 18, 2012 at 10:20 PM, Tim Horton wrote: >> Without looking at the code, I vaguely think that >> CSSPropertyWebkitTextDecorationLine comes from a generated file that might >> not rebuild when it's supposed to. You should try a clean build and see -- >> there've been a few problems with this recently (I ran into it when >> switching flexbox and regions/exclusions on and off a lot a few months ago, >> and a colleague ran into it again recently in a similar situation). > > Is there a way to force the EWS build bots to do a clean build? I'm > frequently obtaining build failures on gtk, mac and win due to this > refuse to rebuild these generated files (ie. > https://bugs.webkit.org/show_bug.cgi?id=94094#c9 ). From the output > you can see that both Source/WebCore/css/CSSValueKeywords.in and > Source/WebCore/css/CSSPropertyNames.in were supposed to generate the > new values, but aren't, causing the failures. It'd be preferable to fix whatever problem exists with the dependencies so that these files are automatically regenerated when needed. Otherwise anyone building on these platforms, including buildbots, will just run in to the issue after you land the patch. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On Sat, Aug 18, 2012 at 10:20 PM, Tim Horton wrote: > Without looking at the code, I vaguely think that > CSSPropertyWebkitTextDecorationLine comes from a generated file that might > not rebuild when it's supposed to. You should try a clean build and see -- > there've been a few problems with this recently (I ran into it when switching > flexbox and regions/exclusions on and off a lot a few months ago, and a > colleague ran into it again recently in a similar situation). Is there a way to force the EWS build bots to do a clean build? I'm frequently obtaining build failures on gtk, mac and win due to this refuse to rebuild these generated files (ie. https://bugs.webkit.org/show_bug.cgi?id=94094#c9 ). From the output you can see that both Source/WebCore/css/CSSValueKeywords.in and Source/WebCore/css/CSSPropertyNames.in were supposed to generate the new values, but aren't, causing the failures. Best regards, -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
ah ok. On Sat, Aug 18, 2012 at 10:06 PM, Tim Horton wrote: > > On Aug 18, 2012, at 8:35 PM, Jake wrote: > > Just a guess but shouldn't the #if line be: > > #if defined(ENABLE_CSS3_TEXT_DECORATION) && > defined(ENABLE_CSS3_TEXT_DECORATION) > > > No, this is supposed to be "if ENABLE_CSS3_TEXT_DECORATION is defined and > it is defined to true". The line is correct as it stands. > > -Jake > > On Sat, Aug 18, 2012 at 8:20 PM, Tim Horton wrote: > >> >> >> On Aug 17, 2012, at 11:57 AM, Bruno Abinader >> wrote: >> >> > On Fri, Aug 17, 2012 at 2:24 PM, Joe Mason wrote: >> >> What do you mean by "precompiler"? The preprocessor? Or is this a >> precompiled headers thing? >> > >> > Indeed, s/precompiler/preprocessor :) >> > >> >> >> >> 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? >> > >> > There is no typo as far as I've checked. The following was added to >> > Source/WebCore/css/CSSPropertyNames.in: >> > >> > ... >> > #if defined(ENABLE_CSS3_TEXT_DECORATION) && ENABLE_CSS3_TEXT_DECORATION >> > -webkit-text-decoration-line >> > #endif >> > ... >> > >> >> 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. >> > >> > +1 on that. I've re-checked the build bot output and it seems the >> > issue is actually caused because CSSPropertyWebkitTextDecorationLine >> > gets undefined. So I'm starting to believe the fail reason is >> > something else skipping from my eyes. >> >> Without looking at the code, I vaguely think that >> CSSPropertyWebkitTextDecorationLine comes from a generated file that might >> not rebuild when it's supposed to. You should try a clean build and see -- >> there've been a few problems with this recently (I ran into it when >> switching flexbox and regions/exclusions on and off a lot a few months ago, >> and a colleague ran into it again recently in a similar situation). >> >> > The actual bug related to this >> > issue is https://bugs.webkit.org/show_bug.cgi?id=94108 . >> > >> >> >> >>> 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. >> > >> > My bad :/ The right bug is 94093: >> > https://bugs.webkit.org/show_bug.cgi?id=94093 . >> > >> > -- >> > Bruno de Oliveira Abinader >> > ___ >> > webkit-dev mailing list >> > webkit-dev@lists.webkit.org >> > http://lists.webkit.org/mailman/listinfo/webkit-dev >> >> ___ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org >> http://lists.webkit.org/mailman/listinfo/webkit-dev >> > > > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On Aug 18, 2012, at 8:35 PM, Jake wrote: > Just a guess but shouldn't the #if line be: > > #if defined(ENABLE_CSS3_TEXT_DECORATION) && > defined(ENABLE_CSS3_TEXT_DECORATION) No, this is supposed to be "if ENABLE_CSS3_TEXT_DECORATION is defined and it is defined to true". The line is correct as it stands. > -Jake > > On Sat, Aug 18, 2012 at 8:20 PM, Tim Horton wrote: > > > On Aug 17, 2012, at 11:57 AM, Bruno Abinader wrote: > > > On Fri, Aug 17, 2012 at 2:24 PM, Joe Mason wrote: > >> What do you mean by "precompiler"? The preprocessor? Or is this a > >> precompiled headers thing? > > > > Indeed, s/precompiler/preprocessor :) > > > >> > >> 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? > > > > There is no typo as far as I've checked. The following was added to > > Source/WebCore/css/CSSPropertyNames.in: > > > > ... > > #if defined(ENABLE_CSS3_TEXT_DECORATION) && ENABLE_CSS3_TEXT_DECORATION > > -webkit-text-decoration-line > > #endif > > ... > > > >> 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. > > > > +1 on that. I've re-checked the build bot output and it seems the > > issue is actually caused because CSSPropertyWebkitTextDecorationLine > > gets undefined. So I'm starting to believe the fail reason is > > something else skipping from my eyes. > > Without looking at the code, I vaguely think that > CSSPropertyWebkitTextDecorationLine comes from a generated file that might > not rebuild when it's supposed to. You should try a clean build and see -- > there've been a few problems with this recently (I ran into it when switching > flexbox and regions/exclusions on and off a lot a few months ago, and a > colleague ran into it again recently in a similar situation). > > > The actual bug related to this > > issue is https://bugs.webkit.org/show_bug.cgi?id=94108 . > > > >> > >>> 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. > > > > My bad :/ The right bug is 94093: > > https://bugs.webkit.org/show_bug.cgi?id=94093 . > > > > -- > > Bruno de Oliveira Abinader > > ___ > > webkit-dev mailing list > > webkit-dev@lists.webkit.org > > http://lists.webkit.org/mailman/listinfo/webkit-dev > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
Just a guess but shouldn't the #if line be: #if defined(ENABLE_CSS3_TEXT_DECORATION) && defined(ENABLE_CSS3_TEXT_DECORATION) -Jake On Sat, Aug 18, 2012 at 8:20 PM, Tim Horton wrote: > > > On Aug 17, 2012, at 11:57 AM, Bruno Abinader > wrote: > > > On Fri, Aug 17, 2012 at 2:24 PM, Joe Mason wrote: > >> What do you mean by "precompiler"? The preprocessor? Or is this a > precompiled headers thing? > > > > Indeed, s/precompiler/preprocessor :) > > > >> > >> 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? > > > > There is no typo as far as I've checked. The following was added to > > Source/WebCore/css/CSSPropertyNames.in: > > > > ... > > #if defined(ENABLE_CSS3_TEXT_DECORATION) && ENABLE_CSS3_TEXT_DECORATION > > -webkit-text-decoration-line > > #endif > > ... > > > >> 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. > > > > +1 on that. I've re-checked the build bot output and it seems the > > issue is actually caused because CSSPropertyWebkitTextDecorationLine > > gets undefined. So I'm starting to believe the fail reason is > > something else skipping from my eyes. > > Without looking at the code, I vaguely think that > CSSPropertyWebkitTextDecorationLine comes from a generated file that might > not rebuild when it's supposed to. You should try a clean build and see -- > there've been a few problems with this recently (I ran into it when > switching flexbox and regions/exclusions on and off a lot a few months ago, > and a colleague ran into it again recently in a similar situation). > > > The actual bug related to this > > issue is https://bugs.webkit.org/show_bug.cgi?id=94108 . > > > >> > >>> 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. > > > > My bad :/ The right bug is 94093: > > https://bugs.webkit.org/show_bug.cgi?id=94093 . > > > > -- > > Bruno de Oliveira Abinader > > ___ > > webkit-dev mailing list > > webkit-dev@lists.webkit.org > > http://lists.webkit.org/mailman/listinfo/webkit-dev > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On Aug 17, 2012, at 11:57 AM, Bruno Abinader wrote: > On Fri, Aug 17, 2012 at 2:24 PM, Joe Mason wrote: >> What do you mean by "precompiler"? The preprocessor? Or is this a >> precompiled headers thing? > > Indeed, s/precompiler/preprocessor :) > >> >> 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? > > There is no typo as far as I've checked. The following was added to > Source/WebCore/css/CSSPropertyNames.in: > > ... > #if defined(ENABLE_CSS3_TEXT_DECORATION) && ENABLE_CSS3_TEXT_DECORATION > -webkit-text-decoration-line > #endif > ... > >> 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. > > +1 on that. I've re-checked the build bot output and it seems the > issue is actually caused because CSSPropertyWebkitTextDecorationLine > gets undefined. So I'm starting to believe the fail reason is > something else skipping from my eyes. Without looking at the code, I vaguely think that CSSPropertyWebkitTextDecorationLine comes from a generated file that might not rebuild when it's supposed to. You should try a clean build and see -- there've been a few problems with this recently (I ran into it when switching flexbox and regions/exclusions on and off a lot a few months ago, and a colleague ran into it again recently in a similar situation). > The actual bug related to this > issue is https://bugs.webkit.org/show_bug.cgi?id=94108 . > >> >>> 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. > > My bad :/ The right bug is 94093: > https://bugs.webkit.org/show_bug.cgi?id=94093 . > > -- > Bruno de Oliveira Abinader > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
On Fri, Aug 17, 2012 at 2:24 PM, Joe Mason wrote: > What do you mean by "precompiler"? The preprocessor? Or is this a > precompiled headers thing? Indeed, s/precompiler/preprocessor :) > > 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? There is no typo as far as I've checked. The following was added to Source/WebCore/css/CSSPropertyNames.in: ... #if defined(ENABLE_CSS3_TEXT_DECORATION) && ENABLE_CSS3_TEXT_DECORATION -webkit-text-decoration-line #endif ... > 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. +1 on that. I've re-checked the build bot output and it seems the issue is actually caused because CSSPropertyWebkitTextDecorationLine gets undefined. So I'm starting to believe the fail reason is something else skipping from my eyes. The actual bug related to this issue is https://bugs.webkit.org/show_bug.cgi?id=94108 . > >> 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. My bad :/ The right bug is 94093: https://bugs.webkit.org/show_bug.cgi?id=94093 . -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Proposal for coding guidelines: Do not use fall-through switch cases inside #ifdef's
> From: webkit-dev-boun...@lists.webkit.org > [webkit-dev-boun...@lists.webkit.org] on behalf of Bruno Abinader > [brunoabina...@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. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[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: ... case CSSPropertyTextDecoration: return renderTextDecorationFlagsToCSSValue(style->textDecoration()); #if ENABLE(CSS3_TEXT_DECORATION) case CSSPropertyWebkitTextDecorationLine: return renderTextDecorationFlagsToCSSValue(style->textDecoration()); #endif // CSS3_TEXT_DECORATION ... 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 propose it as a code guideline. As always, inputs and comments are welcome! Best regards, -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
