[webkit-reviews] review granted: [Bug 97754] Clean up HasTrivialConstructor/Destructor : [Attachment 166024] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 97754: Clean up HasTrivialConstructor/Destructor
https://bugs.webkit.org/show_bug.cgi?id=97754

Attachment 166024: Patch
https://bugs.webkit.org/attachment.cgi?id=166024&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.