Bug 96980 - Use WTF::HasTrivialDestructor instead of compiler-specific versions in JSC::NeedsDestructor
Summary: Use WTF::HasTrivialDestructor instead of compiler-specific versions in JSC::N...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
Depends on:
Blocks: 96546
  Show dependency treegraph
Reported: 2012-09-17 23:02 PDT by Mark Hahnenberg
Modified: 2012-09-18 09:25 PDT (History)
2 users (show)

See Also:

Patch (3.09 KB, patch)
2012-09-17 23:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-09-17 23:02:04 PDT
We should avoid re-inventing the wheel and just use all the good stuff WTF gives to use for free.
Comment 1 Mark Hahnenberg 2012-09-17 23:37:02 PDT
Created attachment 164499 [details]
Comment 2 Benjamin Poulain 2012-09-17 23:48:09 PDT
Comment on attachment 164499 [details]

Looks good to me.
Why not get rid of NeedsDestructor entirely and use HasTrivialDestructor at the two call sites?
Comment 3 Mark Hahnenberg 2012-09-17 23:51:12 PDT
(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 4 WebKit Review Bot 2012-09-18 09:12:48 PDT
Comment on attachment 164499 [details]

Clearing flags on attachment: 164499

Committed r128900: <http://trac.webkit.org/changeset/128900>
Comment 5 WebKit Review Bot 2012-09-18 09:12:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2012-09-18 09:25:02 PDT
Comment on attachment 164499 [details]

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.