Bug 146103 – Refactor CheckedArithmeticOperations.cpp to use templates instead of macros

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mark Lam]

Presently, the tests in CheckedArithmeticOperations.cpp are all implemented as part of a large macro.  This makes them harder to:
1. write: no editor help with indentations, have to add trailing '\'s, inconvenient to add line breaks and comments.
2. read: no chroma coding / syntax highlighting
3. debug: compile time errors are reported as being on the single line where the macro is used.

Refactoring the tests to use C++ templates solves all these issues.

[Mark Lam]

Created attachment 255102 [details]
patch for review.

Note: this patch contains unwanted artifacts that left in for the sole purpose of minimizing the diff so that the patch will be easier to review.  The artifacts are documented in with comments in the patch, and will be removed before landing.

[Basile Clement]

Comment on attachment 255102 [details]
patch for review.

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

Not a reviewer, but I think Coerser should be Coercer throughout the patch.

> Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:36
> +        CheckedArithmeticTester<type, CoercerType, MixedSignednessTesterType>(); \

Shouldn't this be CheckedArithmeticTester<type, CoercerType, MixedSignednessTesterType>::run() ?

[Mark Lam]

(In reply to comment #2)
> Comment on attachment 255102 [details]
> patch for review.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255102&action=review
> 
> Not a reviewer, but I think Coerser should be Coercer throughout the patch.
> 
> > Tools/TestWebKitAPI/Tests/WTF/CheckedArithmeticOperations.cpp:36
> > +        CheckedArithmeticTester<type, CoercerType, MixedSignednessTesterType>(); \
> 
> Shouldn't this be CheckedArithmeticTester<type, CoercerType,
> MixedSignednessTesterType>::run() ?

Eek, you are right.  Fixing immediately.

[Mark Lam]

(In reply to comment #3)
> (In reply to comment #2)
> > Shouldn't this be CheckedArithmeticTester<type, CoercerType,
> > MixedSignednessTesterType>::run() ?
> 
> Eek, you are right.  Fixing immediately.

For the record, I had tested it with CheckedArithmeticTester<type, CoercerType, MixedSignednessTesterType>::run().  However, I had tried to change the patch to use a function template instead of a class template (hence the error you saw), but that turned out to be less readable.  So, I changed it back, but missed that one line.  It is not fixed.

Patch landed in r185708: <http://trac.webkit.org/r185708>.
Followup fix landed in r185711: <http://trac.webkit.org/r185711>.

[Mark Lam]

(In reply to comment #4)
> changed it back, but missed that one line.  It is not fixed.

typo: I meant it is *now* fixed.