Bug 206637

Summary: `FALLTHROUGH` macro isn't properly defined when building Objective-C files using Clang
Product: WebKit Reporter: Conrad Shultz <conrad_shultz>
Component: Web Template FrameworkAssignee: Conrad Shultz <conrad_shultz>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, conrad_shultz, darin, dbates, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Conrad Shultz 2020-01-22 18:29:56 PST 
In the non-C++ case, FALLTHROUGH is defined only when using GCC.
Comment 1 Conrad Shultz 2020-01-22 18:30:34 PST 
<rdar://problem/58807878>
Comment 2 Radar WebKit Bug Importer 2020-01-22 18:30:49 PST 
<rdar://problem/58820324>
Comment 3 Conrad Shultz 2020-01-22 18:32:16 PST 
<rdar://problem/58807878>
Comment 4 Conrad Shultz 2020-01-22 18:36:36 PST 
Created attachment 388506 [details]
Patch
Comment 5 Darin Adler 2020-01-22 20:47:37 PST 
Comment on attachment 388506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388506&action=review

> Source/WTF/wtf/Compiler.h:207
> -#if COMPILER(GCC)
> +#if COMPILER(GCC) || (COMPILER(CLANG) && __has_attribute(fallthrough))

Why doesn’t the section above with __has_cpp_attribute work? I believe that’s what’s supposed to cover this.
Comment 6 Darin Adler 2020-01-22 20:48:02 PST 
Comment on attachment 388506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388506&action=review

>> Source/WTF/wtf/Compiler.h:207
>> +#if COMPILER(GCC) || (COMPILER(CLANG) && __has_attribute(fallthrough))
> 
> Why doesn’t the section above with __has_cpp_attribute work? I believe that’s what’s supposed to cover this.

Oh, non-C++, I see
Comment 7 Darin Adler 2020-01-22 20:48:43 PST 
Comment on attachment 388506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388506&action=review

>>> Source/WTF/wtf/Compiler.h:207
>>> +#if COMPILER(GCC) || (COMPILER(CLANG) && __has_attribute(fallthrough))
>> 
>> Why doesn’t the section above with __has_cpp_attribute work? I believe that’s what’s supposed to cover this.
> 
> Oh, non-C++, I see

Since all the versions of CLANG we compile with have this support, I suggest just writing:

    #if COMPILE(GCC_COMPATIBLE)
Comment 8 Conrad Shultz 2020-01-23 15:07:26 PST 
(In reply to Darin Adler from comment #7)
> Comment on attachment 388506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388506&action=review
> 
> >>> Source/WTF/wtf/Compiler.h:207
> >>> +#if COMPILER(GCC) || (COMPILER(CLANG) && __has_attribute(fallthrough))
> >> 
> >> Why doesn’t the section above with __has_cpp_attribute work? I believe that’s what’s supposed to cover this.
> > 
> > Oh, non-C++, I see
> 
> Since all the versions of CLANG we compile with have this support, I suggest
> just writing:
> 
>     #if COMPILE(GCC_COMPATIBLE)

In my testing, I found this wasn't always true, so I'd prefer to leave the `__has_attribute` check.

Thanks for the review, will sort out what's going on with Windows before landing.
Comment 9 Conrad Shultz 2020-01-28 13:53:46 PST 
Created attachment 389056 [details]
Patch
Comment 10 Conrad Shultz 2020-01-30 11:55:53 PST 
Created attachment 389274 [details]
Patch
Comment 11 WebKit Commit Bot 2020-01-30 16:15:17 PST 
The commit-queue encountered the following flaky tests while processing attachment 389274 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-01-30 16:15:53 PST 
Comment on attachment 389274 [details]
Patch

Clearing flags on attachment: 389274

Committed r255467: <https://trac.webkit.org/changeset/255467>
Comment 13 WebKit Commit Bot 2020-01-30 16:15:55 PST 
All reviewed patches have been landed.  Closing bug.