Bug 73683 - KeyframeValueList::operator= is neither safe to use nor prohibited
Summary: KeyframeValueList::operator= is neither safe to use nor prohibited
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-02 12:30 PST by Yong Li
Modified: 2011-12-05 21:30 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 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.
Comment 1 Yong Li 2011-12-02 12:32:33 PST
Created attachment 117669 [details]
make the operator= private
Comment 2 Darin Adler 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.
Comment 3 Darin Adler 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.
Comment 4 Yong Li 2011-12-05 07:17:16 PST
Created attachment 117874 [details]
Implement operator= with the copy constructor.

How about this? :)
Comment 5 Gyuyoung Kim 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
Comment 6 Early Warning System Bot 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
Comment 7 WebKit Review Bot 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
Comment 8 Yong Li 2011-12-05 09:53:38 PST
Created attachment 117892 [details]
impl operator= with copy constructor
Comment 9 Darin Adler 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.
Comment 10 Yong Li 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.
Comment 11 WebKit Review Bot 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
Comment 12 Yong Li 2011-12-05 14:02:42 PST
Created attachment 117936 [details]
implement operator= with swap()
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2011-12-05 21:30:32 PST
All reviewed patches have been landed.  Closing bug.