bitwise_cast infinite loops if called from the default constructor in ToType

[Saam Barati]

...

[JF Bastien]

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 ;-)

[JF Bastien]

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

[JF Bastien]

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.

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[JF Bastien]

Created attachment 287794 [details]
patch

Fix style.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 287794 [details]
patch

Clearing flags on attachment: 287794

Committed r205362: <http://trac.webkit.org/changeset/205362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.