Summary: | bitwise_cast infinite loops if called from the default constructor in ToType | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
Component: | Web Template Framework | Assignee: | JF Bastien <jfbastien> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, saam | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Saam Barati
2016-08-29 17:22:52 PDT
This is a mild regression from the previous code which didn't call either type's ctor. There's now kind-of an assumption that ToType is default-constructible, and that its default ctor behaves (almost as if it's POD). That's not great. I'll think of a clever C++-ism to fix. Funny that this is the first time someone runs into this issue just after my change ;-) I take back what I said: it wasn't possible before to use bitwise_cast when ToType had a non-trivial ctor (in that case the default ctor of the union is deleted), but could have been made so with a noop ctor in the union: template<typename To, typename From> To bit_cast(From from) { union U { U() {}; To to; From from; } u; u.from = from; return u.to; } struct A { A(int in = 5) { v = in; } operator int() { return v; } int v; }; int main() { return bit_cast<A>(3.14); } This is horrible and should not be done. The standard says: If any non-static data member of a union has a non-trivial default constructor (12.1), copy constructor (12.8), move constructor (12.8), copy assignment operator (12.8), move assignment operator (12.8), or destructor (12.4), the corresponding member function of the union must be user-provided or it will be implicitly deleted (8.4.3) for the union. This is therefore a regression in that the new version of bitwise_cast is *more* permissive because it accepts ToTypes which aren't trivially constructible, but will cause sad recursion when the ToType calls bitwise_cast. Let's think about this some more... Created attachment 287651 [details]
patch
This patch fixes the one issue.
I'm still not happy about From being copied in (it should be an rvalue ref, or at least a const ref), but the build disagrees with me.
I'm working on adding this to the C++ standard library so we'd have this fixed once and for all. I still need to add concepts (instead of static_assert or SFINAE) and make constexpr work (ugh!) but I have some proposed wording and a bunch of tests.
I think fixing this minor issue is good enough for now, and C++20 will have the most wonderful fix.
Attachment 287651 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space before { [whitespace/braces] [5]
ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 2 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287651 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=287651&action=review r=me > Source/WTF/wtf/StdLibExtras.h:154 > + ToType to{}; Nit: I think webkit style is to have "{ }" instead of "{}". It's worth verifying. Created attachment 287794 [details]
patch
Fix style.
Attachment 287794 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space before { [whitespace/braces] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 287794 [details] patch Clearing flags on attachment: 287794 Committed r205362: <http://trac.webkit.org/changeset/205362> All reviewed patches have been landed. Closing bug. |