Bug 188195 – Use static const global variable for TransformationMatrix instead of NeverDestroyed

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

[Yusuke Suzuki]

Use static const global variable for TransformationMatrix instead of NeverDestroyed

[Yusuke Suzuki]

Created attachment 346167 [details]
Patch

[Yusuke Suzuki]

Created attachment 346168 [details]
Patch

[Yusuke Suzuki]

Created attachment 346169 [details]
Patch

[Darin Adler]

Comment on attachment 346169 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
> +static const TransformationMatrix identityTransform { };

The braces here are OK, but not needed.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }

I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".

[Yusuke Suzuki]

Comment on attachment 346169 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>> +static const TransformationMatrix identityTransform { };
> 
> The braces here are OK, but not needed.

Personally, I would like to have this here since it makes explicit that this is a definition :).

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
>> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }
> 
> I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".

In this patch, I keep these things. TransformationMatrix::operator= is different from the default operator: it first checks `*this != other`, and memcpy if the above condition is true.
I'm not sure this check before copying is important... I think it is rather costly. But in the meantime, I would like to keep this patch simple by not having operator=/copy constructor changes.
I'll open a bug to remove operator=/copy constructor separately.

[Yusuke Suzuki]

Committed r234427: <https://trac.webkit.org/changeset/234427>

[Radar WebKit Bug Importer]

<rdar://problem/42774823>

[Yusuke Suzuki]

Comment on attachment 346169 [details]
Patch

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

>>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
>>> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }
>> 
>> I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".
> 
> In this patch, I keep these things. TransformationMatrix::operator= is different from the default operator: it first checks `*this != other`, and memcpy if the above condition is true.
> I'm not sure this check before copying is important... I think it is rather costly. But in the meantime, I would like to keep this patch simple by not having operator=/copy constructor changes.
> I'll open a bug to remove operator=/copy constructor separately.

Ah, no. setMatrix's `if (m && m != m_matrix)` is pointer check since m is a C array. default copy constructor just works as expected.

[Simon Fraser (smfr)]

Comment on attachment 346169 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>>> +static const TransformationMatrix identityTransform { };
>> 
>> The braces here are OK, but not needed.
> 
> Personally, I would like to have this here since it makes explicit that this is a definition :).

Can TransformationMatrix.h just export a static const TransformationMatrix identityTransform?

[Yusuke Suzuki]

Comment on attachment 346169 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>>>> +static const TransformationMatrix identityTransform { };
>>> 
>>> The braces here are OK, but not needed.
>> 
>> Personally, I would like to have this here since it makes explicit that this is a definition :).
> 
> Can TransformationMatrix.h just export a static const TransformationMatrix identityTransform?

That sounds nice. I'll try this.