Bug 146833 - Introduce COMPILER(GCC_OR_CLANG) guard and make COMPILER(GCC) true only for GCC
Summary: Introduce COMPILER(GCC_OR_CLANG) guard and make COMPILER(GCC) true only for GCC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-10 03:52 PDT by Csaba Osztrogonác
Modified: 2015-08-05 09:04 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2015-07-10 03:57 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
test all platforms (1.44 KB, patch)
2015-07-10 04:03 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (35.85 KB, patch)
2015-07-29 03:44 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (35.99 KB, patch)
2015-07-30 11:23 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-07-10 03:52:36 PDT
There are many COMPILER(GCC) || COMPILER(CLANG) guards in the source tree.

COMPILER(CLANG) is technically a subset of COMPILER(GCC), because
COMPILER_GCC is set if __GNUC__ is defined. But it is defined by 
Clang too, so COMPILER(CLANG) is useless with COMPILER(GCC) together.
Comment 1 Csaba Osztrogonác 2015-07-10 03:57:03 PDT
Created attachment 256576 [details]
Patch
Comment 2 Csaba Osztrogonác 2015-07-10 04:03:41 PDT
Created attachment 256577 [details]
test all platforms

patch for testing if it is true on all platforms
Comment 3 WebKit Commit Bot 2015-07-10 04:05:23 PDT
Attachment 256577 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:36:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:36:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:38:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:44:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:46:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 9 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Csaba Osztrogonác 2015-07-10 04:08:11 PDT
Comment on attachment 256577 [details]
test all platforms

confirmed that GCC is always true everywhere if CLANG is true.
Comment 5 Alexey Proskuryakov 2015-07-10 04:50:16 PDT
Comment on attachment 256576 [details]
Patch

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

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:334
> -#if COMPILER(CLANG) || COMPILER(GCC)
> +#if COMPILER(GCC)

The proposed variant looks actively misleading to me, it's hard to expect that the code will not be ifdefed out for clang.

Perhaps it's wrong to have COMPILER(GCC) defined to 1 for clang?
Comment 6 Csaba Osztrogonác 2015-07-10 05:27:02 PDT
(In reply to comment #5)
> Comment on attachment 256576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256576&action=review
> 
> > Source/JavaScriptCore/interpreter/StackVisitor.cpp:334
> > -#if COMPILER(CLANG) || COMPILER(GCC)
> > +#if COMPILER(GCC)
> 
> The proposed variant looks actively misleading to me, it's hard to expect
> that the code will not be ifdefed out for clang.
> 
> Perhaps it's wrong to have COMPILER(GCC) defined to 1 for clang?

We can change COMPILER(GCC) to false for Clang ... but it would mean 
that we should change COMPILER(GCC) to COMPILER(GCC) || COMPILER(CLANG)
near ~100 places. I don't like this idea at all.
Comment 7 Darin Adler 2015-07-10 09:50:25 PDT
Comment on attachment 256576 [details]
Patch

In active ports, who is using a compiler other then clang or MSVC? Which active port is using non-clang gcc?
Comment 8 Csaba Osztrogonác 2015-07-10 09:53:22 PDT
(In reply to comment #7)
> Comment on attachment 256576 [details]
> Patch
> 
> In active ports, who is using a compiler other then clang or MSVC? Which
> active port is using non-clang gcc?

EFL and GTK use GCC (min. 4.9) as the default system 
compiler, but both of them build fine with Clang too.
Comment 9 Darin Adler 2015-07-22 13:49:16 PDT
Comment on attachment 256576 [details]
Patch

I think the principled way to do this would be to come up with a name for “both gcc and clang” other than “gcc”. Then we could do a global replace of COMPILER(GCC) with this new name, then apply tis patch.
Comment 10 Csaba Osztrogonác 2015-07-29 03:44:30 PDT
Created attachment 257744 [details]
Patch

patch for EWS
Comment 11 WebKit Commit Bot 2015-07-29 03:47:32 PDT
Attachment 257744 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/StdLibExtras.h:41:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Csaba Osztrogonác 2015-07-30 11:23:10 PDT
Created attachment 257843 [details]
Patch
Comment 13 WebKit Commit Bot 2015-07-30 11:25:38 PDT
Attachment 257843 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/StdLibExtras.h:41:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Csaba Osztrogonác 2015-07-30 11:31:53 PDT
(In reply to comment #9)
> Comment on attachment 256576 [details]
> Patch
> 
> I think the principled way to do this would be to come up with a name for
> “both gcc and clang” other than “gcc”. Then we could do a global replace of
> COMPILER(GCC) with this new name, then apply tis patch.

(In reply to comment #12)
> Created attachment 257843 [details]
> Patch

I renamed COMPILER(GCC) to COMPILER(GCC_OR_CLANG) 
and did the cleanup in this patch too.
Comment 15 WebKit Commit Bot 2015-07-30 14:22:06 PDT
Attachment 257843 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/StdLibExtras.h:41:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Csaba Osztrogonác 2015-08-03 06:17:50 PDT
ping?
Comment 17 WebKit Commit Bot 2015-08-03 23:51:41 PDT
Comment on attachment 257843 [details]
Patch

Clearing flags on attachment: 257843

Committed r187819: <http://trac.webkit.org/changeset/187819>
Comment 18 WebKit Commit Bot 2015-08-03 23:51:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Simon Fraser (smfr) 2015-08-04 12:04:19 PDT
A JSC test started to fail around the time this landed, and this looks like the most likely cause:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29
Comment 20 Csaba Osztrogonác 2015-08-05 00:42:46 PDT
(In reply to comment #19)
> A JSC test started to fail around the time this landed, and this looks like
> the most likely cause:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Debug%20JSC%20%28Tests%29

The bot is green nowadays, I think bug147142 was the culprit for this 
regression too. It was already rolled out, but I'll keep my eyes on
the bot in these days.
Comment 21 Alexey Proskuryakov 2015-08-05 09:04:11 PDT
Turned out that the JSC failure on raytrace test started before this patch, it's flaky.