Bug 67095 – Add support for checked arithmetic

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Add support for checked arithmetic

[Oliver Hunt]

Created attachment 105449 [details]
Patch

[WebKit Review Bot]

Attachment 105449 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/CheckedArithmetic.h:201:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:232:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:263:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:271:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:279:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:494:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 105449 [details]
Patch

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

It would be awesome if this had tests in TestWebKitAPI.

> Source/JavaScriptCore/wtf/CheckedArithmetic.h:90
> +    void overflowed()
> +    {
> +        m_overflowed = true;
> +    }

I think this would read cleaner if it had the word set in it.

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

This is quite odd that it is an unsigned char here and not a bool.  If you need this for some reason, it should be well documented.

[Oliver Hunt]

Created attachment 105678 [details]
Patch

[WebKit Review Bot]

Attachment 105678 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/CheckedArithmetic.h:207:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:223:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:238:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:269:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:278:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:287:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:509:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Created attachment 105708 [details]
Patch

[WebKit Review Bot]

Attachment 105708 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/CheckedArithmetic.h:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:258:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:273:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:304:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:313:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:322:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:544:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Created attachment 105713 [details]
Patch

[WebKit Review Bot]

Attachment 105713 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/CheckedArithmetic.h:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:258:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:273:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:304:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:313:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:322:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/wtf/CheckedArithmetic.h:544:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 105713 [details]
Patch

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.

[Oliver Hunt]

Committed r94207: <http://trac.webkit.org/changeset/94207>

[Adam Roben (:aroben)]

Comment on attachment 105713 [details]
Patch

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

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:24
> +		A7E69B5E140C117200223412 /* CheckedArithmeticOperations.cpp in Sources */ = {isa = PBXBuildFile; fileRef = A7E69B5D140C117200223412 /* CheckedArithmeticOperations.cpp */; };

Can you add this to TestWebKitAPI.vcproj, too?

[Oliver Hunt]

(In reply to comment #12)
> (From update of attachment 105713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105713&action=review
> 
> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:24
> > +		A7E69B5E140C117200223412 /* CheckedArithmeticOperations.cpp in Sources */ = {isa = PBXBuildFile; fileRef = A7E69B5D140C117200223412 /* CheckedArithmeticOperations.cpp */; };
> 
> Can you add this to TestWebKitAPI.vcproj, too?

Dammit, i thought i had, but landed now