73683 2011-12-02 12:30:03 -0800 KeyframeValueList::operator= is neither safe to use nor prohibited 2011-12-05 21:30:32 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P5 Trivial --- 1 yong.li.webkit webkit-unassigned darin dglazkov webkit.review.bot oldest_to_newest 514008 0 yong.li.webkit 2011-12-02 12:30:03 -0800 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. 514011 1 117669 yong.li.webkit 2011-12-02 12:32:33 -0800 Created attachment 117669 make the operator= private 514153 2 117669 darin 2011-12-02 14:26:31 -0800 Comment on attachment 117669 make the operator= private This change is OK, but it would also be easy to write a swap and implement the canonical operator= instead. 514155 3 darin 2011-12-02 14:27:29 -0800 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. 515061 4 117874 yong.li.webkit 2011-12-05 07:17:16 -0800 Created attachment 117874 Implement operator= with the copy constructor. How about this? :) 515064 5 117874 gyuyoung.kim 2011-12-05 07:25:00 -0800 Comment on attachment 117874 Implement operator= with the copy constructor. Attachment 117874 did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10732638 515069 6 117874 webkit-ews 2011-12-05 07:30:04 -0800 Comment on attachment 117874 Implement operator= with the copy constructor. Attachment 117874 did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10736319 515079 7 117874 webkit.review.bot 2011-12-05 07:51:49 -0800 Comment on attachment 117874 Implement operator= with the copy constructor. Attachment 117874 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10734669 515155 8 117892 yong.li.webkit 2011-12-05 09:53:38 -0800 Created attachment 117892 impl operator= with copy constructor 515239 9 117892 darin 2011-12-05 11:26:37 -0800 Comment on attachment 117892 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. 515355 10 yong.li.webkit 2011-12-05 13:32:35 -0800 (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. 515368 11 117892 webkit.review.bot 2011-12-05 13:50:56 -0800 Comment on attachment 117892 impl operator= with copy constructor Attachment 117892 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10727906 New failing tests: svg/custom/linking-uri-01-b.svg 515376 12 117936 yong.li.webkit 2011-12-05 14:02:42 -0800 Created attachment 117936 implement operator= with swap() 515634 13 117936 webkit.review.bot 2011-12-05 21:30:26 -0800 Comment on attachment 117936 implement operator= with swap() Clearing flags on attachment: 117936 Committed r102092: <http://trac.webkit.org/changeset/102092> 515635 14 webkit.review.bot 2011-12-05 21:30:32 -0800 All reviewed patches have been landed. Closing bug. 117669 2011-12-02 12:32:33 -0800 2011-12-05 14:28:59 -0800 make the operator= private 73683.diff text/plain 1086 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA5MjI4OGIyLi5jYmU2MjNmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMTItMDIgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczNjgzCisgICAgICAgIE1ha2UgS2V5 ZnJhbWVWYWx1ZUxpc3Q6Om9wZXJhdG9yPSBwcml2YXRlIGJlY2F1c2UgaXQgaXMgbm90IHNhZmUg dG8gdXNlLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE5vIG5ldyB0ZXN0cyBhcyBubyBmdW5jdGlvbmFsIGNoYW5nZS4KKworICAgICAgICAqIHBsYXRm b3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIuaDogTWFrZSBLZXlmcmFtZVZhbHVlTGlzdDo6b3Bl cmF0b3I9IHByaXZhdGUuCisKIDIwMTEtMTItMDIgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJldmll dy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHIxMDE4 MzMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9HcmFwaGlj c0xheWVyLmggYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9HcmFwaGljc0xheWVy LmgKaW5kZXggN2I4MzQwZS4uMTZiYWFkMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MvR3JhcGhpY3NMYXllci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIuaApAQCAtMTgyLDYgKzE4Miw5IEBAIHB1YmxpYzoK IHByb3RlY3RlZDoKICAgICBWZWN0b3I8Y29uc3QgQW5pbWF0aW9uVmFsdWUqPiBtX3ZhbHVlczsK ICAgICBBbmltYXRlZFByb3BlcnR5SUQgbV9wcm9wZXJ0eTsKKworcHJpdmF0ZToKKyAgICBLZXlm cmFtZVZhbHVlTGlzdCYgb3BlcmF0b3I9KGNvbnN0IEtleWZyYW1lVmFsdWVMaXN0Jik7CiB9Owog CiAK 117874 2011-12-05 07:17:16 -0800 2011-12-05 09:02:21 -0800 Implement operator= with the copy constructor. 73683-imp.diff text/plain 1282 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA5MjI4OGIyLi41ODQ4ZGNlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMTItMDIgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczNjgzCisgICAgICAgIEltcGxlbWVu dCBLZXlmcmFtZVZhbHVlTGlzdDo6b3BlcmF0b3I9IHdpdGggaXRzIGNvcHkgY29uc3RydWN0b3Iu CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzIGFzIG5vIGZ1bmN0aW9uYWwgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3MvR3JhcGhpY3NMYXllci5oOiBNYWtlIEtleWZyYW1lVmFsdWVMaXN0OjpvcGVyYXRvcj0g cHJpdmF0ZS4KKwogMjAxMS0xMi0wMiAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBn bWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjEwMTgzMy4KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIu aCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIuaAppbmRl eCA3YjgzNDBlLi4yOGFjYzk3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9HcmFwaGljc0xheWVyLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvR3JhcGhpY3NMYXllci5oCkBAIC0xNzAsNyArMTcwLDE2IEBAIHB1YmxpYzoKICAgICB7 CiAgICAgICAgIGRlbGV0ZUFsbFZhbHVlcyhtX3ZhbHVlcyk7CiAgICAgfQotICAgIAorCisgICAg S2V5ZnJhbWVWYWx1ZUxpc3QmIG9wZXJhdG9yPShjb25zdCBLZXlmcmFtZVZhbHVlTGlzdCYgbykK KyAgICB7CisgICAgICAgIGlmICh0aGlzID09ICZvKQorICAgICAgICAgICAgcmV0dXJuOworCisg ICAgICAgIH5LZXlmcmFtZVZhbHVlTGlzdCgpOworICAgICAgICBuZXcgKHRoaXMpIEtleWZyYW1l VmFsdWVMaXN0KG8pOworICAgIH0KKwogICAgIEFuaW1hdGVkUHJvcGVydHlJRCBwcm9wZXJ0eSgp IGNvbnN0IHsgcmV0dXJuIG1fcHJvcGVydHk7IH0KIAogICAgIHNpemVfdCBzaXplKCkgY29uc3Qg eyByZXR1cm4gbV92YWx1ZXMuc2l6ZSgpOyB9Cg== 117892 2011-12-05 09:53:38 -0800 2011-12-05 14:29:07 -0800 impl operator= with copy constructor 73683-imp.diff text/plain 1317 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA5MjI4OGIyLi41ODQ4ZGNlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMTItMDIgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczNjgzCisgICAgICAgIEltcGxlbWVu dCBLZXlmcmFtZVZhbHVlTGlzdDo6b3BlcmF0b3I9IHdpdGggaXRzIGNvcHkgY29uc3RydWN0b3Iu CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzIGFzIG5vIGZ1bmN0aW9uYWwgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3MvR3JhcGhpY3NMYXllci5oOiBNYWtlIEtleWZyYW1lVmFsdWVMaXN0OjpvcGVyYXRvcj0g cHJpdmF0ZS4KKwogMjAxMS0xMi0wMiAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBn bWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjEwMTgzMy4KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIu aCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIuaAppbmRl eCA3YjgzNDBlLi43NWQzYWY4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9HcmFwaGljc0xheWVyLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvR3JhcGhpY3NMYXllci5oCkBAIC0xNzAsNyArMTcwLDE3IEBAIHB1YmxpYzoKICAgICB7 CiAgICAgICAgIGRlbGV0ZUFsbFZhbHVlcyhtX3ZhbHVlcyk7CiAgICAgfQotICAgIAorCisgICAg S2V5ZnJhbWVWYWx1ZUxpc3QmIG9wZXJhdG9yPShjb25zdCBLZXlmcmFtZVZhbHVlTGlzdCYgbykK KyAgICB7CisgICAgICAgIGlmICh0aGlzID09ICZvKQorICAgICAgICAgICAgcmV0dXJuICp0aGlz OworCisgICAgICAgIHRoaXMtPn5LZXlmcmFtZVZhbHVlTGlzdCgpOworICAgICAgICBuZXcgKHRo aXMpIEtleWZyYW1lVmFsdWVMaXN0KG8pOworICAgICAgICByZXR1cm4gKnRoaXM7CisgICAgfQor CiAgICAgQW5pbWF0ZWRQcm9wZXJ0eUlEIHByb3BlcnR5KCkgY29uc3QgeyByZXR1cm4gbV9wcm9w ZXJ0eTsgfQogCiAgICAgc2l6ZV90IHNpemUoKSBjb25zdCB7IHJldHVybiBtX3ZhbHVlcy5zaXpl KCk7IH0K 117936 2011-12-05 14:02:42 -0800 2011-12-05 21:30:26 -0800 implement operator= with swap() 73683-imp.diff text/plain 1467 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA5MjI4OGIyLi42MTJiMzI0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMTItMDIgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczNjgzCisgICAgICAgIEltcGxlbWVu dCBLZXlmcmFtZVZhbHVlTGlzdDo6b3BlcmF0b3I9KCkgYW5kIEtleWZyYW1lVmFsdWVMaXN0Ojpz d2FwKCkuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg Tm8gbmV3IHRlc3RzIGFzIG5vIGZ1bmN0aW9uYWwgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvR3JhcGhpY3NMYXllci5oOiAKKyAgICAgICAgKFdlYkNvcmU6OktleWZyYW1l VmFsdWVMaXN0OjpvcGVyYXRvcj0pOiBBZGRlZAorICAgICAgICAoV2ViQ29yZTo6S2V5ZnJhbWVW YWx1ZUxpc3Q6OnN3YXApOiBBZGRlZAorCiAyMDExLTEyLTAyICBTaGVyaWZmIEJvdCAgPHdlYmtp dC5yZXZpZXcuYm90QGdtYWlsLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91 dCByMTAxODMzLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv R3JhcGhpY3NMYXllci5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvR3JhcGhp Y3NMYXllci5oCmluZGV4IDdiODM0MGUuLjBhMTgxODkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL0dyYXBoaWNzTGF5ZXIuaAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy9HcmFwaGljc0xheWVyLmgKQEAgLTE3MCw3ICsxNzAsMjAgQEAg cHVibGljOgogICAgIHsKICAgICAgICAgZGVsZXRlQWxsVmFsdWVzKG1fdmFsdWVzKTsKICAgICB9 Ci0gICAgCisKKyAgICBLZXlmcmFtZVZhbHVlTGlzdCYgb3BlcmF0b3I9KGNvbnN0IEtleWZyYW1l VmFsdWVMaXN0JiBvdGhlcikKKyAgICB7CisgICAgICAgIEtleWZyYW1lVmFsdWVMaXN0IGNvcHko b3RoZXIpOworICAgICAgICBzd2FwKGNvcHkpOworICAgICAgICByZXR1cm4gKnRoaXM7CisgICAg fQorCisgICAgdm9pZCBzd2FwKEtleWZyYW1lVmFsdWVMaXN0JiBvdGhlcikKKyAgICB7CisgICAg ICAgIHN0ZDo6c3dhcChtX3Byb3BlcnR5LCBvdGhlci5tX3Byb3BlcnR5KTsKKyAgICAgICAgbV92 YWx1ZXMuc3dhcChvdGhlci5tX3ZhbHVlcyk7CisgICAgfQorCiAgICAgQW5pbWF0ZWRQcm9wZXJ0 eUlEIHByb3BlcnR5KCkgY29uc3QgeyByZXR1cm4gbV9wcm9wZXJ0eTsgfQogCiAgICAgc2l6ZV90 IHNpemUoKSBjb25zdCB7IHJldHVybiBtX3ZhbHVlcy5zaXplKCk7IH0K