Bug 97754 – Clean up HasTrivialConstructor/Destructor

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

[Mark Hahnenberg]

As noted in bug 96980, TypeTraits.h could use a little cleaning up.

[Mark Hahnenberg]

Created attachment 165921 [details]
Patch

[Mark Hahnenberg]

Created attachment 166019 [details]
Patch

[Mark Hahnenberg]

Created attachment 166024 [details]
Patch

[Darin Adler]

Comment on attachment 166024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166024&action=review

> Source/WTF/ChangeLog:3
> +        Cleanup HasTrivialConstructor/Destructor

The verb “clean up” is two words.

> Source/WTF/wtf/TypeTraits.h:244
>      // VC8 (VS2005) and later have built-in compiler support for HasTrivialConstructor / HasTrivialDestructor,

I don’t think it’s right to say they have built-in compiler support for HasTrivialConstructor / HasTrivialDestructor. The comment should say:

    // VC8 (VS2005) and later has __has_trivial_constructor and __has_trivial_destructor, but the implementation
    // incorrectly returns false for built-in types. Work around that by explicitly checking IsPod.

Also, if we wanted to cut down on copies of these templates, we could consider merging this case with the clang case. I see no harm in including the Visual Studio workaround even in the compilers that don’t need it.

> Source/WTF/wtf/TypeTraits.h:245
> +    // but for some unexplained reason it doesn't work on built-in types. We add the extra IsPod condition to 

I don’t think the phrase “for some unexplained reason” added anything to the old comment and adds nothing to the new either.

It would be nice if the comment said something more specific than “it doesn’t work on” like “it returns false for”.

> Source/WTF/wtf/TypeTraits.h:255
> +    // For platforms that don't support HasTrivialConstructor/HasTrivialDestructor at all, we want to at least provide 
> +    // some semblance of support.

I think the comment should be more specific. I would have written something more like this:

    // For compilers that don't support detection of trivial constructors and destructors in classes, use a template
    // that returns true for any POD type but false for all class types. This will give false negatives, which can
    // hurt performance, but avoids false positives, which can result in incorrect behavior.

[Mark Hahnenberg]

Committed r129780: <http://trac.webkit.org/changeset/129780>

[Darin Adler]

(In reply to comment #4)
> Also, if we wanted to cut down on copies of these templates, we could consider merging this case with the clang case. I see no harm in including the Visual Studio workaround even in the compilers that don’t need it.

Oops, my suggestion probably broke the VC10 case. It’s supposed to use std::tr1::has_trivial_constructor, right?

[Mark Hahnenberg]

> Oops, my suggestion probably broke the VC10 case. It’s supposed to use std::tr1::has_trivial_constructor, right?

Argh, I believe so. Reopening...

[Mark Hahnenberg]

Created attachment 166095 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166095 [details]
Patch

Clearing flags on attachment: 166095

Committed r129999: <http://trac.webkit.org/changeset/129999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.