We should avoid re-inventing the wheel and just use all the good stuff WTF gives to use for free.
Created attachment 164499 [details] Patch
Comment on attachment 164499 [details] Patch Looks good to me. Why not get rid of NeedsDestructor entirely and use HasTrivialDestructor at the two call sites?
(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.
Comment on attachment 164499 [details] Patch Clearing flags on attachment: 164499 Committed r128900: <http://trac.webkit.org/changeset/128900>
All reviewed patches have been landed. Closing bug.
Comment on attachment 164499 [details] 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.