161365 2016-08-29 17:22:52 -0700 bitwise_cast infinite loops if called from the default constructor in ToType 2016-09-02 12:41:17 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 saam jfbastien benjamin cdumez cmarcelo commit-queue dbates saam oldest_to_newest 1224458 0 saam 2016-08-29 17:22:52 -0700 ... 1224463 1 jfbastien 2016-08-29 17:27:04 -0700 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 ;-) 1224474 2 jfbastien 2016-08-29 17:49:38 -0700 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... 1225444 3 287651 jfbastien 2016-09-01 12:00:37 -0700 Created attachment 287651 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. 1225446 4 commit-queue 2016-09-01 12:02:45 -0700 Attachment 287651 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. 1225603 5 287651 saam 2016-09-01 16:18:39 -0700 Comment on attachment 287651 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. 1225872 6 287794 jfbastien 2016-09-02 12:09:45 -0700 Created attachment 287794 patch Fix style. 1225875 7 commit-queue 2016-09-02 12:11:39 -0700 Attachment 287794 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. 1225894 8 287794 commit-queue 2016-09-02 12:40:56 -0700 Comment on attachment 287794 patch Clearing flags on attachment: 287794 Committed r205362: <http://trac.webkit.org/changeset/205362> 1225895 9 commit-queue 2016-09-02 12:41:17 -0700 All reviewed patches have been landed. Closing bug. 287651 2016-09-01 12:00:37 -0700 2016-09-02 12:09:45 -0700 patch 0001-aggregate-init.patch text/plain 1522 jfbastien RnJvbSA4NTFkNmE2OTgwM2Q1YTU0NjM0YmJjZTI1Nzc5NTFjNjZlY2M0YjU4IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKRiBCYXN0aWVuIDxqZmJhc3RpZW5AYXBwbGUuY29tPgpEYXRl OiBUaHUsIDEgU2VwIDIwMTYgMTE6NTU6MjUgLTA3MDAKU3ViamVjdDogW1BBVENIXSBhZ2dyZWdh dGUgaW5pdAoKLS0tCiBTb3VyY2UvV1RGL0NoYW5nZUxvZyAgICAgICAgICB8IDEwICsrKysrKysr KysKIFNvdXJjZS9XVEYvd3RmL1N0ZExpYkV4dHJhcy5oIHwgIDIgKy0KIDIgZmlsZXMgY2hhbmdl ZCwgMTEgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9X VEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKaW5kZXggYjkxMzAwMi4uNDI1ZGZh YyAxMDA2NDQKLS0tIGEvU291cmNlL1dURi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dURi9DaGFu Z2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAxNi0wOS0wMSAgSkYgQmFzdGllbiAgPGpmYmFzdGll bkBhcHBsZS5jb20+CisKKyAgICAgICAgYml0d2lzZV9jYXN0IGluZmluaXRlIGxvb3BzIGlmIGNh bGxlZCBmcm9tIHRoZSBkZWZhdWx0IGNvbnN0cnVjdG9yIGluIFRvVHlwZQorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTYxMzY1CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiB3dGYvU3RkTGliRXh0cmFzLmg6 CisgICAgICAgIChXVEY6OmJpdHdpc2VfY2FzdCk6IHVzZSBhZ2dyZWdhdGUgaW5pdGlhbGl6YXRp b24gdG8gYXZvaWQgY3RvcgorCiAyMDE2LTA4LTMxICBLZWl0aCBSb2xsaW4gIDxrcm9sbGluQGFw cGxlLmNvbT4KIAogICAgICAgICBXZWJLaXQgc2hvdWxkIHNldCBhIHN1YnN5c3RlbSBmb3Igb3Nf bG9nIHNvIGl0J3MgZWFzaWVyIHRvIGZpbHRlciBmb3IgV2ViS2l0IGxvZyBtZXNzYWdlcwpkaWZm IC0tZ2l0IGEvU291cmNlL1dURi93dGYvU3RkTGliRXh0cmFzLmggYi9Tb3VyY2UvV1RGL3d0Zi9T dGRMaWJFeHRyYXMuaAppbmRleCAzMGRlNTkzLi4wMWE5MmMwIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V1RGL3d0Zi9TdGRMaWJFeHRyYXMuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9TdGRMaWJFeHRyYXMu aApAQCAtMTUxLDcgKzE1MSw3IEBAIGlubGluZSBUb1R5cGUgYml0d2lzZV9jYXN0KEZyb21UeXBl IGZyb20pCiAgICAgc3RhdGljX2Fzc2VydChfX2lzX3RyaXZpYWxseV9jb3B5YWJsZShUb1R5cGUp LCAiYml0d2lzZV9jYXN0IG9mIG5vbi10cml2aWFsbHktY29weWFibGUgdHlwZSEiKTsKICAgICBz dGF0aWNfYXNzZXJ0KF9faXNfdHJpdmlhbGx5X2NvcHlhYmxlKEZyb21UeXBlKSwgImJpdHdpc2Vf Y2FzdCBvZiBub24tdHJpdmlhbGx5LWNvcHlhYmxlIHR5cGUhIik7CiAjZW5kaWYKLSAgICBUb1R5 cGUgdG87CisgICAgVG9UeXBlIHRve307CiAgICAgc3RkOjptZW1jcHkoJnRvLCAmZnJvbSwgc2l6 ZW9mKHRvKSk7CiAgICAgcmV0dXJuIHRvOwogfQotLSAKMi45LjMKCg== 287794 2016-09-02 12:09:45 -0700 2016-09-02 12:40:56 -0700 patch 0001-aggregate-init.patch text/plain 1517 jfbastien RnJvbSA5NzdiYTYxZWJjYmE3NTJiNDdkZTVkOWFiZjBlNDhmNTNhYWU0ZTZhIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKRiBCYXN0aWVuIDxqZmJhc3RpZW5AYXBwbGUuY29tPgpEYXRl OiBUaHUsIDEgU2VwIDIwMTYgMTE6NTU6MjUgLTA3MDAKU3ViamVjdDogW1BBVENIXSBhZ2dyZWdh dGUgaW5pdAoKLS0tCiBTb3VyY2UvV1RGL0NoYW5nZUxvZyAgICAgICAgICB8IDEwICsrKysrKysr KysKIFNvdXJjZS9XVEYvd3RmL1N0ZExpYkV4dHJhcy5oIHwgIDIgKy0KIDIgZmlsZXMgY2hhbmdl ZCwgMTEgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9X VEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKaW5kZXggMTU1OTg0Ni4uYTcxZTAw ZDkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTYtMDktMDEgIEpGIEJhc3RpZW4gIDxqZmJhc3Rp ZW5AYXBwbGUuY29tPgorCisgICAgICAgIGJpdHdpc2VfY2FzdCBpbmZpbml0ZSBsb29wcyBpZiBj YWxsZWQgZnJvbSB0aGUgZGVmYXVsdCBjb25zdHJ1Y3RvciBpbiBUb1R5cGUKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MTM2NQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL1N0ZExpYkV4dHJhcy5o OgorICAgICAgICAoV1RGOjpiaXR3aXNlX2Nhc3QpOiB1c2UgYWdncmVnYXRlIGluaXRpYWxpemF0 aW9uIHRvIGF2b2lkIGN0b3IKKwogMjAxNi0wOS0wMSAgQW5kZXJzIENhcmxzc29uICA8YW5kZXJz Y2FAYXBwbGUuY29tPgogCiAgICAgICAgIFVzZSBCbG9ja1B0cjo6ZnJvbUNhbGxhYmxlIGluIFdv cmtRdWV1ZTo6ZGlzcGF0Y2ggYW5kIFdvcmtRdWV1ZTo6ZGlzcGF0Y2hBZnRlcgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dURi93dGYvU3RkTGliRXh0cmFzLmggYi9Tb3VyY2UvV1RGL3d0Zi9TdGRMaWJF eHRyYXMuaAppbmRleCAzMGRlNTkzLi4xNGFiMzgxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0 Zi9TdGRMaWJFeHRyYXMuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9TdGRMaWJFeHRyYXMuaApAQCAt MTUxLDcgKzE1MSw3IEBAIGlubGluZSBUb1R5cGUgYml0d2lzZV9jYXN0KEZyb21UeXBlIGZyb20p CiAgICAgc3RhdGljX2Fzc2VydChfX2lzX3RyaXZpYWxseV9jb3B5YWJsZShUb1R5cGUpLCAiYml0 d2lzZV9jYXN0IG9mIG5vbi10cml2aWFsbHktY29weWFibGUgdHlwZSEiKTsKICAgICBzdGF0aWNf YXNzZXJ0KF9faXNfdHJpdmlhbGx5X2NvcHlhYmxlKEZyb21UeXBlKSwgImJpdHdpc2VfY2FzdCBv ZiBub24tdHJpdmlhbGx5LWNvcHlhYmxlIHR5cGUhIik7CiAjZW5kaWYKLSAgICBUb1R5cGUgdG87 CisgICAgVG9UeXBlIHRveyB9OwogICAgIHN0ZDo6bWVtY3B5KCZ0bywgJmZyb20sIHNpemVvZih0 bykpOwogICAgIHJldHVybiB0bzsKIH0KLS0gCjIuOS4zCgo=