Bug 146833 – Introduce COMPILER(GCC_OR_CLANG) guard and make COMPILER(GCC) true only for GCC

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 256576 [details]
Patch

[Csaba Osztrogonác]

Created attachment 256577 [details]
test all platforms

patch for testing if it is true on all platforms

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

Comment on attachment 256577 [details]
test all platforms

confirmed that GCC is always true everywhere if CLANG is true.

[Alexey Proskuryakov]

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?

[Csaba Osztrogonác]

(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.

[Darin Adler]

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?

[Csaba Osztrogonác]

(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.

[Darin Adler]

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.

[Csaba Osztrogonác]

Created attachment 257744 [details]
Patch

patch for EWS

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

Created attachment 257843 [details]
Patch

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

(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.

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

ping?

[WebKit Commit Bot]

Comment on attachment 257843 [details]
Patch

Clearing flags on attachment: 257843

Committed r187819: <http://trac.webkit.org/changeset/187819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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

[Csaba Osztrogonác]

(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.

[Alexey Proskuryakov]

Turned out that the JSC failure on raytrace test started before this patch, it's flaky.