Bug 67095 - Add support for checked arithmetic
Summary: Add support for checked arithmetic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-28 12:57 PDT by Oliver Hunt
Modified: 2011-08-31 16:03 PDT (History)
2 users (show)

See Also:

Attachments
Patch (78.85 KB, patch)
2011-08-28 13:09 PDT, Oliver Hunt 		no flags Details | Formatted Diff | Diff
Patch (63.85 KB, patch)
2011-08-30 13:04 PDT, Oliver Hunt 		no flags Details | Formatted Diff | Diff
Patch (65.14 KB, patch)
2011-08-30 15:32 PDT, Oliver Hunt 		no flags Details | Formatted Diff | Diff
Patch (65.17 KB, patch)
2011-08-30 15:59 PDT, Oliver Hunt 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-08-28 12:57:30 PDT 
Add support for checked arithmetic
Comment 1 Oliver Hunt 2011-08-28 13:09:46 PDT 
Created attachment 105449 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-28 13:12:37 PDT 
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.
Comment 3 Sam Weinig 2011-08-28 16:42:25 PDT 
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.
Comment 4 Oliver Hunt 2011-08-30 13:04:23 PDT 
Created attachment 105678 [details]
Patch
Comment 5 WebKit Review Bot 2011-08-30 13:07:25 PDT 
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.
Comment 6 Oliver Hunt 2011-08-30 15:32:01 PDT 
Created attachment 105708 [details]
Patch
Comment 7 WebKit Review Bot 2011-08-30 15:34:53 PDT 
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.
Comment 8 Oliver Hunt 2011-08-30 15:59:06 PDT 
Created attachment 105713 [details]
Patch
Comment 9 WebKit Review Bot 2011-08-30 16:01:11 PDT 
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.
Comment 10 Sam Weinig 2011-08-30 19:33:27 PDT 
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.
Comment 11 Oliver Hunt 2011-08-31 11:16:07 PDT 
Committed r94207: <http://trac.webkit.org/changeset/94207>
Comment 12 Adam Roben (:aroben) 2011-08-31 11:22:25 PDT 
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?
Comment 13 Oliver Hunt 2011-08-31 16:03:52 PDT 
(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