Summary: | Introduce COMPILER(GCC_OR_CLANG) guard and make COMPILER(GCC) true only for GCC | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||
Component: | New Bugs | Assignee: | Csaba Osztrogonác <ossy> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, cmarcelo, commit-queue, darin, dino, kondapallykalyan, ossy, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2015-07-10 03:52:36 PDT
Created attachment 256576 [details]
Patch
Created attachment 256577 [details]
test all platforms
patch for testing if it is true on all platforms
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 on attachment 256577 [details]
test all platforms
confirmed that GCC is always true everywhere if CLANG is true.
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? (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 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?
(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 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.
Created attachment 257744 [details]
Patch
patch for EWS
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.
Created attachment 257843 [details]
Patch
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.
(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. 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.
ping? Comment on attachment 257843 [details] Patch Clearing flags on attachment: 257843 Committed r187819: <http://trac.webkit.org/changeset/187819> All reviewed patches have been landed. Closing bug. 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 (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. Turned out that the JSC failure on raytrace test started before this patch, it's flaky. |