Summary: | KeyframeValueList::operator= is neither safe to use nor prohibited | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Trivial | CC: | darin, dglazkov, webkit.review.bot | ||||||||||
Priority: | P5 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Yong Li
2011-12-02 12:30:03 PST
Created attachment 117669 [details]
make the operator= private
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.
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. Created attachment 117874 [details]
Implement operator= with the copy constructor.
How about this? :)
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 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 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 Created attachment 117892 [details]
impl operator= with copy constructor
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. (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 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 Created attachment 117936 [details]
implement operator= with swap()
Comment on attachment 117936 [details] implement operator= with swap() Clearing flags on attachment: 117936 Committed r102092: <http://trac.webkit.org/changeset/102092> All reviewed patches have been landed. Closing bug. |