[webkit-reviews] review granted: [Bug 67095] Add support for checked arithmetic : [Attachment 105713] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 67095: Add support for checked arithmetic
https://bugs.webkit.org/show_bug.cgi?id=67095

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105713&action=review


> Source/JavaScriptCore/wtf/CheckedArithmetic.h:101
> +    unsigned char m_overflowed;

Please put a comment here about why you are using unsigned char rather than
bool.

> Source/JavaScriptCore/wtf/CheckedArithmetic.h:161
> +template <typename U, typename V> struct TypeCompare {
> +    static const bool SameType = false;
> +};
> +
> +template <typename U> struct TypeCompare<U, U> {
> +    static const bool SameType = true;
> +};

I believe this is already provided in wtf/TypeTraits.h as struct IsSameType<T,
U>.

>> Source/JavaScriptCore/wtf/CheckedArithmetic.h:242
>> +	{
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

The style queue is wrong here, and a bug should should be file on it.

> Source/JavaScriptCore/wtf/CheckedArithmetic.h:532
> +	       CRASH();
> +	       return (m_value) ? reinterpret_cast<UnspecifiedBoolType*>(1) :
0;

Indentation here is wrong.

> Tools/ChangeLog:11
> +	   * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
> +	   * TestWebKitAPI/Tests/CheckedArithmeticOperations.cpp: Added.

You should probably also add the test to the vcproj.