Summary: | bitwise_cast uses inactive member of union | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | JF Bastien <jfbastien> | ||||||||||||||
Component: | Web Template Framework | Assignee: | JF Bastien <jfbastien> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap, benjamin, buildbot, cdumez, cmarcelo, commit-queue, darin, dbates, rniwa | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
JF Bastien
2016-08-26 08:04:53 PDT
Created attachment 287104 [details]
patch
Created attachment 287109 [details]
patch
Updated patch with COMPILER_HAS_CLANG_FEATURE.
Attachment 287109 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Compiler.h:46: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287109 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=287109&action=review > Source/WTF/wtf/StdLibExtras.h:149 > +# if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable) Style: don't indent #if > Source/WTF/wtf/StdLibExtras.h:153 > +# endif ditto Created attachment 287121 [details]
patch
Fix style.
Attachment 287121 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Compiler.h:46: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287121 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=287121&action=review > Source/WTF/ChangeLog:7 > + I think it's worth putting the paragraph of explanation here that you have in bugzilla. That text is informative, and it's the exact kind of thing that we want in a changelog. Comment on attachment 287121 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=287121&action=review > Source/WTF/wtf/StdLibExtras.h:149 > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable) How did you come to the decision to add his new macro function as opposed to using COMPLILER_SUPPORTS()? My understanding of the latter macro function is that it allows checking for a compiler feature in a way that is compiler agnostic. At the time of writing, COMPILER_SUPPORTS() is only Implemented to return a non-undefined value for Clang. I do not see why we cannot extend this macro function to work with other toolchains. (In reply to comment #8) > Comment on attachment 287121 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287121&action=review > > > Source/WTF/wtf/StdLibExtras.h:149 > > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable) > > How did you come to the decision to add his new macro function as opposed to > using COMPLILER_SUPPORTS()? My understanding of the latter macro function is > that it allows checking for a compiler feature in a way that is compiler > agnostic. At the time of writing, COMPILER_SUPPORTS() is only > Implemented to return a non-undefined value for Clang. I do not see why we > cannot extend this macro function to work with other toolchains. __has_feature is clang's favorite way of declaring what it supports. It has a bunch of options, so I think it's inconvenient to define all the COMPILER_SUPPORTS macros in Compiler.h and then use them were needed (Ideally I'd just modify StdLibExtras.h in this patch). It's also much nicer to do: #if I_CAN_HAZ(...) // Kittens! #endif Rather than: #if defined(I_CAN_HAZ) && I_CAN_HAZ(...) // Kittens! #endif The former is the COMPILER_HAS_CLANG_FEATURE usage, whereas the latter is the COMPILER_SUPPORTS usage. The C++ standard also has recommended "feature testing" support which GCC and clang have started to support, which is very similar to __has_feature. It's unfortunately not uniformly supported by standard library implementations. I think What I've done is therefore pretty idiomatic C++ and will become more so in the future. I'd move *away* from COMPILER_SUPPORTS and instead use COMPILER_HAS_CLANG_FEATURE and feature testing :-) But hey I'm new here, just a C++ dork doing what seems obvious, LMK if it's odd (I mean, besides C++ being odd in general). Created attachment 287137 [details]
patch
Move bugzilla comment to ChangeLog, and enhance it with references to the C++ Standard which back up my claims.
Attachment 287137 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Compiler.h:46: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 287121 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=287121&action=review > > > > > Source/WTF/wtf/StdLibExtras.h:149 > > > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable) > > > > How did you come to the decision to add his new macro function as opposed to > > using COMPLILER_SUPPORTS()? My understanding of the latter macro function is > > that it allows checking for a compiler feature in a way that is compiler > > agnostic. At the time of writing, COMPILER_SUPPORTS() is only > > Implemented to return a non-undefined value for Clang. I do not see why we > > cannot extend this macro function to work with other toolchains. > > __has_feature is clang's favorite way of declaring what it supports. It has > a bunch of options, so I think it's inconvenient to define all the > COMPILER_SUPPORTS macros in Compiler.h and then use them were needed > (Ideally I'd just modify StdLibExtras.h in this patch). > With the exception of WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS we seem to names these macro defines following the same naming as the Clang feature. I do not see why we should adopt such a convention. The COMPILER_SUPPORTS() macro gives us an opportunity to come up with names that are not Clang-specific and may even better describe the feature they are tested, much like how FALLTHROUGH_WARNINGS is more descriptive than its implementation. > It's also much nicer to do: > #if I_CAN_HAZ(...) > // Kittens! > #endif > Rather than: > #if defined(I_CAN_HAZ) && I_CAN_HAZ(...) > // Kittens! > #endif > The former is the COMPILER_HAS_CLANG_FEATURE usage, whereas the latter is > the COMPILER_SUPPORTS usage. > Notice that COMPILER_SUPPORTS(X) is a macro function that takes care of this expansion, <https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h#L33>.2 > The C++ standard also has recommended "feature testing" support which GCC > and clang have started to support, which is very similar to __has_feature. > It's unfortunately not uniformly supported by standard library > implementations. > Then would it not make sense to use COMPILER_SUPPORTS() so that one day we can implement the WTF_COMPILER_SUPPORTS_X macros in terms of the C++ standard machinery?This has the benefit that all compliant compilers will make use of such macro-guarded code without the need to change each call site. > I think What I've done is therefore pretty idiomatic C++ and will become > more so in the future. I'd move *away* from COMPILER_SUPPORTS and instead > use COMPILER_HAS_CLANG_FEATURE and feature testing :-) > > But hey I'm new here, just a C++ dork doing what seems obvious, LMK if it's > odd (I mean, besides C++ being odd in general). (In reply to comment #8) > Comment on attachment 287121 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287121&action=review > > > Source/WTF/wtf/StdLibExtras.h:149 > > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable) > > How did you come to the decision to add his new macro function as opposed to > using COMPLILER_SUPPORTS()? My understanding of the latter macro function is > that it allows checking for a compiler feature in a way that is compiler > agnostic. At the time of writing, COMPILER_SUPPORTS() is only > Implemented to return a non-undefined value for Clang. I do not see why we > cannot extend this macro function to work with other toolchains. Comment on attachment 287137 [details] patch Attachment 287137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1949082 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html Created attachment 287148 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
The imported/w3c/web-platform-tests/html/dom/interfaces.html failure is tracked in bug 161142. I always wondered where the recommendation to use unions for type casting came from. I didn't get it first hand, but it seemed like the recommended way to work around strict aliasing issues. > With the exception of WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS we seem to > names these macro defines following the same naming as the Clang feature. I > do not see why we should adopt such a convention. The COMPILER_SUPPORTS() > macro gives us an opportunity to come up with names that are not > Clang-specific and may even better describe the feature they are tested, > much like how FALLTHROUGH_WARNINGS is more descriptive than its > implementation. Ah you're right, I'd misunderstood. I've updated the patch to use COMPILER_SUPPORTS. > Then would it not make sense to use COMPILER_SUPPORTS() so that one day we > can implement the WTF_COMPILER_SUPPORTS_X macros in terms of the C++ > standard machinery?This has the benefit that all compliant compilers will > make use of such macro-guarded code without the need to change each call > site. Indeed. Created attachment 287185 [details]
patch
Update macros.
Attachment 287185 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Compiler.h:46: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287185 [details]
patch
LGTM.
Looks like the WK2 bot is just broken at the moment.
Comment on attachment 287185 [details] patch Clearing flags on attachment: 287185 Committed r205066: <http://trac.webkit.org/changeset/205066> All reviewed patches have been landed. Closing bug. |