WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
73683
KeyframeValueList::operator= is neither safe to use nor prohibited
https://bugs.webkit.org/show_bug.cgi?id=73683
Summary
KeyframeValueList::operator= is neither safe to use nor prohibited
Yong Li
Reported
2011-12-02 12:30:03 PST
When looking at GraphicLayer.h, I noticed that KeyframeValueList is copyable now, and it has a safe copy constructor. However, its default operator= is still unsafe. We should either implement an operator= or simply disable it. Not a big deal. But it would be nice we include something like this in webkit coding guidelines: when implement a copy constructor, please also implement operator=, and vice versa.
Attachments
make the operator= private
(1.06 KB, patch)
2011-12-02 12:32 PST
,
Yong Li
darin
: review+
Details
Formatted Diff
Diff
Implement operator= with the copy constructor.
(1.25 KB, patch)
2011-12-05 07:17 PST
,
Yong Li
gyuyoung.kim
: commit-queue-
Details
Formatted Diff
Diff
impl operator= with copy constructor
(1.29 KB, patch)
2011-12-05 09:53 PST
,
Yong Li
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
implement operator= with swap()
(1.43 KB, patch)
2011-12-05 14:02 PST
,
Yong Li
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Yong Li
Comment 1
2011-12-02 12:32:33 PST
Created
attachment 117669
[details]
make the operator= private
Darin Adler
Comment 2
2011-12-02 14:26:31 PST
Comment on
attachment 117669
[details]
make the operator= private This change is OK, but it would also be easy to write a swap and implement the canonical operator= instead.
Darin Adler
Comment 3
2011-12-02 14:27:29 PST
The canonical one is: KeyframeValueList& operator=(const KeyframeValueList& other) { KeyframeValueList copy(other); swap(copy); return *this; } Assuming we write a swap function that swaps each member.
Yong Li
Comment 4
2011-12-05 07:17:16 PST
Created
attachment 117874
[details]
Implement operator= with the copy constructor. How about this? :)
Gyuyoung Kim
Comment 5
2011-12-05 07:25:00 PST
Comment on
attachment 117874
[details]
Implement operator= with the copy constructor.
Attachment 117874
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/10732638
Early Warning System Bot
Comment 6
2011-12-05 07:30:04 PST
Comment on
attachment 117874
[details]
Implement operator= with the copy constructor.
Attachment 117874
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/10736319
WebKit Review Bot
Comment 7
2011-12-05 07:51:49 PST
Comment on
attachment 117874
[details]
Implement operator= with the copy constructor.
Attachment 117874
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10734669
Yong Li
Comment 8
2011-12-05 09:53:38 PST
Created
attachment 117892
[details]
impl operator= with copy constructor
Darin Adler
Comment 9
2011-12-05 11:26:37 PST
Comment on
attachment 117892
[details]
impl operator= with copy constructor View in context:
https://bugs.webkit.org/attachment.cgi?id=117892&action=review
> Source/WebCore/platform/graphics/GraphicsLayer.h:182 > + KeyframeValueList& operator=(const KeyframeValueList& o) > + { > + if (this == &o) > + return *this; > + > + this->~KeyframeValueList(); > + new (this) KeyframeValueList(o); > + return *this; > + }
The copy and swap idiom is usually preferred over this idiom. I’d like to use that instead.
Yong Li
Comment 10
2011-12-05 13:32:35 PST
(In reply to
comment #9
)
> (From update of
attachment 117892
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=117892&action=review
> > > Source/WebCore/platform/graphics/GraphicsLayer.h:182 > > + KeyframeValueList& operator=(const KeyframeValueList& o) > > + { > > + if (this == &o) > > + return *this; > > + > > + this->~KeyframeValueList(); > > + new (this) KeyframeValueList(o); > > + return *this; > > + } > > The copy and swap idiom is usually preferred over this idiom. I’d like to use that instead.
Copy and swap performs one more copy if not optimized by compiler, and it also needs a swap method. It is first time I got the dtor/copy ctor idiom when writing operator=, and I love it immediately. I even hope it could be a standard that all c++ compilers should implicitly implement operator= in this way when a copy ctor is available: destruct the object and copy-construct a new one on the zombie. One of the pros of the copy and swap idiom is it doesn't need to check (this == &o) assuming swap() already checks that. Also, I cannot find any "new (this)" in current webkit code, which may a little bit tricky. So I'll upload a copy&swap version.
WebKit Review Bot
Comment 11
2011-12-05 13:50:56 PST
Comment on
attachment 117892
[details]
impl operator= with copy constructor
Attachment 117892
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10727906
New failing tests: svg/custom/linking-uri-01-b.svg
Yong Li
Comment 12
2011-12-05 14:02:42 PST
Created
attachment 117936
[details]
implement operator= with swap()
WebKit Review Bot
Comment 13
2011-12-05 21:30:26 PST
Comment on
attachment 117936
[details]
implement operator= with swap() Clearing flags on attachment: 117936 Committed
r102092
: <
http://trac.webkit.org/changeset/102092
>
WebKit Review Bot
Comment 14
2011-12-05 21:30:32 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug