Clean up TransformationMatrix implementation
Created attachment 346173 [details] Patch
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.
Created attachment 346174 [details] Patch
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.
Committed r234433: <https://trac.webkit.org/changeset/234433>
<rdar://problem/42780654>
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.
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.
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.
Committed r234493: <https://trac.webkit.org/changeset/234493>
(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.