Bug 96980 – Use WTF::HasTrivialDestructor instead of compiler-specific versions in JSC::NeedsDestructor

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

We should avoid re-inventing the wheel and just use all the good stuff WTF gives to use for free.

[Mark Hahnenberg]

Created attachment 164499 [details]
Patch

[Benjamin Poulain]

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?

[Mark Hahnenberg]

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

[WebKit Review Bot]

Comment on attachment 164499 [details]
Patch

Clearing flags on attachment: 164499

Committed r128900: <http://trac.webkit.org/changeset/128900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.