Use static const global variable for TransformationMatrix instead of NeverDestroyed
Created attachment 346167 [details] Patch
Created attachment 346168 [details] Patch
Created attachment 346169 [details] Patch
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".
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.
Committed r234427: <https://trac.webkit.org/changeset/234427>
<rdar://problem/42774823>
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.
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?
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.