Bug 188197 – Clean up TransformationMatrix implementation

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Clean up TransformationMatrix implementation

[Yusuke Suzuki]

Created attachment 346173 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:338:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


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

[Yusuke Suzuki]

Created attachment 346174 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:338:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


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

[Yusuke Suzuki]

Committed r234433: <https://trac.webkit.org/changeset/234433>

[Radar WebKit Bug Importer]

<rdar://problem/42780654>

[Darin Adler]

Comment on attachment 346174 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:338
> -        return (m_matrix[0][0] == m2.m_matrix[0][0] &&
> -                m_matrix[0][1] == m2.m_matrix[0][1] &&
> -                m_matrix[0][2] == m2.m_matrix[0][2] &&
> -                m_matrix[0][3] == m2.m_matrix[0][3] &&
> -                m_matrix[1][0] == m2.m_matrix[1][0] &&
> -                m_matrix[1][1] == m2.m_matrix[1][1] &&
> -                m_matrix[1][2] == m2.m_matrix[1][2] &&
> -                m_matrix[1][3] == m2.m_matrix[1][3] &&
> -                m_matrix[2][0] == m2.m_matrix[2][0] &&
> -                m_matrix[2][1] == m2.m_matrix[2][1] &&
> -                m_matrix[2][2] == m2.m_matrix[2][2] &&
> -                m_matrix[2][3] == m2.m_matrix[2][3] &&
> -                m_matrix[3][0] == m2.m_matrix[3][0] &&
> -                m_matrix[3][1] == m2.m_matrix[3][1] &&
> -                m_matrix[3][2] == m2.m_matrix[3][2] &&
> -                m_matrix[3][3] == m2.m_matrix[3][3]);
> +        return memcmp(&m_matrix[0][0], &m2.m_matrix[0][0], sizeof(Matrix4)) == 0;

This is a semantic change for two reasons:

1) double == double and memcmp of the double memory are not the same for numbers like NANs for example.
2) This now potentially compares padding bytes that could have uninitialized data in them; in practice I suppose those won’t be present since doubles are so big.

[Yusuke Suzuki]

Comment on attachment 346174 [details]
Patch

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

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:338
>> +        return memcmp(&m_matrix[0][0], &m2.m_matrix[0][0], sizeof(Matrix4)) == 0;
> 
> This is a semantic change for two reasons:
> 
> 1) double == double and memcmp of the double memory are not the same for numbers like NANs for example.
> 2) This now potentially compares padding bytes that could have uninitialized data in them; in practice I suppose those won’t be present since doubles are so big.

Nice catch! Oops, yeah. I'll revert this part.

[Yusuke Suzuki]

Comment on attachment 346174 [details]
Patch

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

>>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:338
>>> +        return memcmp(&m_matrix[0][0], &m2.m_matrix[0][0], sizeof(Matrix4)) == 0;
>> 
>> This is a semantic change for two reasons:
>> 
>> 1) double == double and memcmp of the double memory are not the same for numbers like NANs for example.
>> 2) This now potentially compares padding bytes that could have uninitialized data in them; in practice I suppose those won’t be present since doubles are so big.
> 
> Nice catch! Oops, yeah. I'll revert this part.

I think this (2) never happens since TransformationMatrix (and Matrix4 m_matrix member) does not have padding.

[Yusuke Suzuki]

Committed r234493: <https://trac.webkit.org/changeset/234493>

[Darin Adler]

(In reply to Yusuke Suzuki from comment #9)
> I think this (2) never happens since TransformationMatrix (and Matrix4
> m_matrix member) does not have padding.

Yes, in practice there is no padding. I was just pointing out that the language doesn’t guarantee that.