96980 2012-09-17 23:02:04 -0700 Use WTF::HasTrivialDestructor instead of compiler-specific versions in JSC::NeedsDestructor 2012-09-18 09:25:02 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 96546 1 mhahnenberg mhahnenberg benjamin webkit.review.bot oldest_to_newest 722501 0 mhahnenberg 2012-09-17 23:02:04 -0700 We should avoid re-inventing the wheel and just use all the good stuff WTF gives to use for free. 722514 1 164499 mhahnenberg 2012-09-17 23:37:02 -0700 Created attachment 164499 Patch 722521 2 164499 benjamin 2012-09-17 23:48:09 -0700 Comment on attachment 164499 Patch Looks good to me. Why not get rid of NeedsDestructor entirely and use HasTrivialDestructor at the two call sites? 722524 3 mhahnenberg 2012-09-17 23:51:12 -0700 (In reply to comment #2) > (From update of attachment 164499 [details]) > Looks good to me. > Why not get rid of NeedsDestructor entirely and use HasTrivialDestructor at the two call sites? A couple reasons. This is more descriptive of what it means in a JSC-related context. I'm also planning on using it a lot more in a future patch (actually, in a patch that just got rolled out :-/ ). Several classes will override their default value in the patch, so it'll be useful to have a separate template for specializing on those classes. 722845 4 164499 webkit.review.bot 2012-09-18 09:12:48 -0700 Comment on attachment 164499 Patch Clearing flags on attachment: 164499 Committed r128900: <http://trac.webkit.org/changeset/128900> 722846 5 webkit.review.bot 2012-09-18 09:12:50 -0700 All reviewed patches have been landed. Closing bug. 722858 6 164499 darin 2012-09-18 09:25:02 -0700 Comment on attachment 164499 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164499&action=review I know this already landed, but I have a few thoughts. > Source/JavaScriptCore/ChangeLog:9 > + (JSC): Strange stray line in change log. > Source/JavaScriptCore/runtime/JSCell.h:314 > + static const bool value = !WTF::HasTrivialDestructor<T>::value; Surprising that you have to specify WTF:: explicitly. Normally the WTF header would have a "using WTF::HasTrivialDestructor" at the bottom of the header file so you would not need it. > Source/WTF/ChangeLog:9 > + (WTF): Another strange stray line left in change log. > Source/WTF/wtf/TypeTraits.h:256 > -#if defined(_MSC_VER) && (_MSC_VER >= 1400) && !defined(__INTEL_COMPILER) > +#if COMPILER(CLANG) || (defined(_MSC_VER) && (_MSC_VER >= 1400) && !defined(__INTEL_COMPILER)) > // VC8 (VS2005) and later have built-in compiler support for HasTrivialConstructor / HasTrivialDestructor, > // but for some unexplained reason it doesn't work on built-in types. The comment is now out of sync with the #if statement. Maybe it’s OK, but I think it’s starting to get confusing. The specializations of HasTrivialConstructor/Destructor are there for two reasons: 1) To make the template return true for built-in types even on compilers that don’t support __has_trivial_constructor/destructor. 2) To work around the problem in Visual Studio. But this comment does not make that clear. Useful comments would make clear that: - This #if statement should be improved over time so we use __has_trivial_constructor/destructor every where it works well enough. - The specializations are needed for the Microsoft compiler to work around a bug in its __has_trivial_constructor/destructor. - This code will conservatively give use false for HasTrivialConstructor/Destructor in compilers that don’t support __has_trivial_constructor/destructor, which is OK although less optimal. Having the comment about the Microsoft compiler inside the #if makes it quite confusing. This file also does a number of strange things, indicating it was written by someone unfamiliar with WebKit coding style: - It omits spaces after false_type and true_type and before "{". - It uses the name signed int for int, and signed short for short, and many others like that. And it’s far more complicated than it needs to be: - It lists all the built-in types separately, but this is unneeded since we have other templates that answer the question “is this a built in type” and can be used here. - It does not use the standard techniques to strip const and volatile from the type and so has to have 4 specializations for every built in type. And it also doesn’t seem to have regression tests. 164499 2012-09-17 23:37:02 -0700 2012-09-18 09:25:02 -0700 Patch bug-96980-20120917233629.patch text/plain 3167 mhahnenberg SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTI4ODUyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDEyLTA5LTE3ICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisK KyAgICAgICAgVXNlIFdURjo6SGFzVHJpdmlhbERlc3RydWN0b3IgaW5zdGVhZCBvZiBjb21waWxl ci1zcGVjaWZpYyB2ZXJzaW9ucyBpbiBKU0M6Ok5lZWRzRGVzdHJ1Y3RvcgorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTY5ODAKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvSlNDZWxsLmg6Cisg ICAgICAgIChKU0MpOgorICAgICAgICAoTmVlZHNEZXN0cnVjdG9yKToKKwogMjAxMi0wOS0xNyAg Q3NhYmEgT3N6dHJvZ29uw6FjICA8b3NzeUB3ZWJraXQub3JnPgogCiAgICAgICAgIFVucmV2aWV3 ZWQsIHJvbGxpbmcgb3V0IHIxMjg4MjYgYW5kIHIxMjg4MTMuCkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvcnVudGltZS9KU0NlbGwuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlw dENvcmUvcnVudGltZS9KU0NlbGwuaAkocmV2aXNpb24gMTI4ODUxKQorKysgU291cmNlL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvSlNDZWxsLmgJKHdvcmtpbmcgY29weSkKQEAgLTMzLDYgKzMzLDcg QEAKICNpbmNsdWRlICJTbG90VmlzaXRvcklubGluZU1ldGhvZHMuaCIKICNpbmNsdWRlICJXcml0 ZUJhcnJpZXIuaCIKICNpbmNsdWRlIDx3dGYvTm9uY29weWFibGUuaD4KKyNpbmNsdWRlIDx3dGYv VHlwZVRyYWl0cy5oPgogCiBuYW1lc3BhY2UgSlNDIHsKIApAQCAtMzA4LDE4ICszMDksMTAgQEAg bmFtZXNwYWNlIEpTQyB7CiAgICAgICAgIHJldHVybiBpc0NlbGwoKSA/IGFzQ2VsbCgpLT50b09i amVjdChleGVjLCBnbG9iYWxPYmplY3QpIDogdG9PYmplY3RTbG93Q2FzZShleGVjLCBnbG9iYWxP YmplY3QpOwogICAgIH0KIAotI2lmIENPTVBJTEVSKENMQU5HKQogICAgIHRlbXBsYXRlPGNsYXNz IFQ+CiAgICAgc3RydWN0IE5lZWRzRGVzdHJ1Y3RvciB7Ci0gICAgICAgIHN0YXRpYyBjb25zdCBi b29sIHZhbHVlID0gIV9faGFzX3RyaXZpYWxfZGVzdHJ1Y3RvcihUKTsKKyAgICAgICAgc3RhdGlj IGNvbnN0IGJvb2wgdmFsdWUgPSAhV1RGOjpIYXNUcml2aWFsRGVzdHJ1Y3RvcjxUPjo6dmFsdWU7 CiAgICAgfTsKLSNlbHNlCi0gICAgLy8gV3JpdGUgbWFudWFsIHNwZWNpYWxpemF0aW9ucyBmb3Ig dGhpcyBzdHJ1Y3QgdGVtcGxhdGUgaWYgeW91IGNhcmUgYWJvdXQgbm9uLWNsYW5nIGNvbXBpbGVy cy4KLSAgICB0ZW1wbGF0ZTxjbGFzcyBUPgotICAgIHN0cnVjdCBOZWVkc0Rlc3RydWN0b3Igewot ICAgICAgICBzdGF0aWMgY29uc3QgYm9vbCB2YWx1ZSA9IHRydWU7Ci0gICAgfTsKLSNlbmRpZgog CiAgICAgdGVtcGxhdGU8dHlwZW5hbWUgVD4KICAgICB2b2lkKiBhbGxvY2F0ZUNlbGwoSGVhcCYg aGVhcCkKSW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X VEYvQ2hhbmdlTG9nCShyZXZpc2lvbiAxMjg4NTIpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwko d29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEyLTA5LTE3ICBNYXJrIEhhaG5lbmJl cmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisKKyAgICAgICAgVXNlIFdURjo6SGFzVHJpdmlh bERlc3RydWN0b3IgaW5zdGVhZCBvZiBjb21waWxlci1zcGVjaWZpYyB2ZXJzaW9ucyBpbiBKU0M6 Ok5lZWRzRGVzdHJ1Y3RvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9OTY5ODAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIHd0Zi9UeXBlVHJhaXRzLmg6CisgICAgICAgIChXVEYpOgorCiAyMDEyLTA5LTE3 ICBHbGVubiBBZGFtcyAgPGdsZW5uQHNreW5hdi5jb20+CiAKICAgICAgICAgSW5jbHVkaW5nIEhl eE51bWJlci5oIGZhaWxzIGJ1aWxkIGlmIGhleERpZ2l0c0Zvck1vZGUgaXMgbm90IHJlZmVyZW5j ZWQKSW5kZXg6IFNvdXJjZS9XVEYvd3RmL1R5cGVUcmFpdHMuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV1RGL3d0Zi9UeXBlVHJhaXRzLmgJKHJldmlzaW9uIDEyODg1MSkKKysrIFNvdXJjZS9XVEYv d3RmL1R5cGVUcmFpdHMuaAkod29ya2luZyBjb3B5KQpAQCAtMjUxLDcgKzI1MSw3IEBAIG5hbWVz cGFjZSBXVEYgewogICAgIHR5cGVkZWYgSW50ZWdyYWxDb25zdGFudDxib29sLCB0cnVlPiAgdHJ1 ZV90eXBlOwogICAgIHR5cGVkZWYgSW50ZWdyYWxDb25zdGFudDxib29sLCBmYWxzZT4gZmFsc2Vf dHlwZTsKIAotI2lmIGRlZmluZWQoX01TQ19WRVIpICYmIChfTVNDX1ZFUiA+PSAxNDAwKSAmJiAh ZGVmaW5lZChfX0lOVEVMX0NPTVBJTEVSKQorI2lmIENPTVBJTEVSKENMQU5HKSB8fCAoZGVmaW5l ZChfTVNDX1ZFUikgJiYgKF9NU0NfVkVSID49IDE0MDApICYmICFkZWZpbmVkKF9fSU5URUxfQ09N UElMRVIpKQogICAgIC8vIFZDOCAoVlMyMDA1KSBhbmQgbGF0ZXIgaGF2ZSBidWlsdC1pbiBjb21w aWxlciBzdXBwb3J0IGZvciBIYXNUcml2aWFsQ29uc3RydWN0b3IgLyBIYXNUcml2aWFsRGVzdHJ1 Y3RvciwKICAgICAvLyBidXQgZm9yIHNvbWUgdW5leHBsYWluZWQgcmVhc29uIGl0IGRvZXNuJ3Qg d29yayBvbiBidWlsdC1pbiB0eXBlcy4KICAgICB0ZW1wbGF0ZSA8dHlwZW5hbWUgVD4gc3RydWN0 IEhhc1RyaXZpYWxDb25zdHJ1Y3RvciA6IHB1YmxpYyBJbnRlZ3JhbENvbnN0YW50PGJvb2wsIF9f aGFzX3RyaXZpYWxfY29uc3RydWN0b3IoVCk+eyB9Owo=