141935 2015-02-23 16:47:39 -0800 WTF::WeakPtr should rename 'forget' to 'clear', and support assignment from nullptr 2015-02-25 15:28:18 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 141923 1 bfulgham bfulgham andersca ap benjamin bfulgham cdumez cmarcelo commit-queue darin rniwa oldest_to_newest 1071412 0 bfulgham 2015-02-23 16:47:39 -0800 After some bike shedding, we decided that: 1. The new "forget" method added in Bug 141923 should be renamed "clear". 2. We really should support assignment from nullptr. 1071421 1 247168 bfulgham 2015-02-23 16:56:21 -0800 Created attachment 247168 Patch 1071423 2 247168 mmaxfield 2015-02-23 16:59:42 -0800 Comment on attachment 247168 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247168&action=review > Source/WTF/wtf/WeakPtr.h:103 > + WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; } Is it valuable to use the argument here? (for performance, obviously correctness is right) > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:183 > + weakPtr7 = nullptr; I wish there was a way we could test compilation failures, so we could make sure that operator=() never works with raw pointers :( 1071427 3 bfulgham 2015-02-23 17:02:46 -0800 (In reply to comment #2) > Comment on attachment 247168 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247168&action=review > > > Source/WTF/wtf/WeakPtr.h:103 > > + WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; } > > Is it valuable to use the argument here? (for performance, obviously > correctness is right) I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-) > > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:183 > > + weakPtr7 = nullptr; > > I wish there was a way we could test compilation failures, so we could make > sure that operator=() never works with raw pointers :( I did try adding this to the end of the WeakPtr.cpp test file: int doNotCopyMe = 152; weakPtr7 = &doNotCopyMe; The compiler properly complained: /Volumes/Data/Projects/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:187:14: error: no viable overloaded '=' weakPtr7 = &doNotCopyMe; ~~~~~~~~ ^ ~~~~~~~~~~~~ In file included from /Volumes/Data/Projects/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:29: /Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:101:14: note: candidate function not viable: no known conversion from 'int *' to 'const WTF::WeakPtr<int>' for 1st argument WeakPtr& operator=(const WeakPtr& o) { m_ref = o.m_ref.copyRef(); return *this; } ^ /Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:103:14: note: candidate function not viable: no known conversion from 'int *' to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; } ^ /Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h:102:35: note: candidate template ignored: could not match 'WeakPtr<type-parameter-0-0>' against 'int *' template<typename U> WeakPtr& operator=(const WeakPtr<U>& o) { m_ref = o.m_ref.copyRef(); return *this; } ^ 1 error generated. ** BUILD FAILED ** 1071428 4 247168 bfulgham 2015-02-23 17:03:49 -0800 Comment on attachment 247168 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247168&action=review >>> Source/WTF/wtf/WeakPtr.h:103 >>> + WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; } >> >> Is it valuable to use the argument here? (for performance, obviously correctness is right) > > I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-) I caught Anders in the hallway, and he say NO NEED! 1071429 5 bfulgham 2015-02-23 17:04:11 -0800 Committed r180535: <http://trac.webkit.org/changeset/180535> 1071471 6 ap 2015-02-23 18:22:35 -0800 This broke the build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/3236/steps/compile-webkit/logs/stdio 1071474 7 rniwa 2015-02-23 18:34:46 -0800 (In reply to comment #6) > This broke the build: > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/ > builds/3236/steps/compile-webkit/logs/stdio Fixed in https://trac.webkit.org/changeset/180540. 1071668 8 andersca 2015-02-24 08:51:24 -0800 (In reply to comment #4) > Comment on attachment 247168 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247168&action=review > > >>> Source/WTF/wtf/WeakPtr.h:103 > >>> + WeakPtr& operator=(std::nullptr_t) { m_ref = WeakReference<T>::create(nullptr); return *this; } > >> > >> Is it valuable to use the argument here? (for performance, obviously correctness is right) > > > > I'm not sure. Maybe Anders can tell us... But since he suggested the implementation I used, I sort of doubt that it would be better ;-) > > I caught Anders in the hallway, and he say NO NEED! To clarify, since nullptr_t is an "empty type", meaning that it has no members etc, it won't even be passed in a register or on the stack! It's only used for overload resolution. 1071912 9 darin 2015-02-24 19:51:18 -0800 Can we just dump clear? 1071913 10 darin 2015-02-24 19:51:31 -0800 Seems like "= nullptr" is sufficient. 1071922 11 bfulgham 2015-02-24 19:59:00 -0800 (In reply to comment #10) > Seems like "= nullptr" is sufficient. Yes, but the other smart pointer implementations do provide a 'clear' method. 1071923 12 cdumez 2015-02-24 20:02:55 -0800 (In reply to comment #11) > (In reply to comment #10) > > Seems like "= nullptr" is sufficient. > > Yes, but the other smart pointer implementations do provide a 'clear' method. But we prefer "= nullptr;" in new code I believe, for consistency with std::unique_ptr. 1072161 13 darin 2015-02-25 15:28:18 -0800 (In reply to comment #11) > Yes, but the other smart pointer implementations do provide a 'clear' method. Ours do, but std::unique_ptr does not. I think eventually we will want to remove our clear functions. We made those back when there was no nullptr in the language. 247168 2015-02-23 16:56:21 -0800 2015-02-23 16:59:42 -0800 Patch bug-141935-20150223165610.patch text/plain 4779 bfulgham SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxODA1MzMpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE1LTAyLTIzICBCcmVudCBGdWxnaGFtICA8YmZ1 bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFdURjo6V2Vha1B0ciBzaG91bGQgcmVuYW1lICdm b3Jnb3QnIHRvICdjbGVhcicgYW5kIHN1cHBvcnQgbnVsbHB0ciBhc3NpZ25tZW50CisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDE5MzUKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHd0Zi9XZWFrUHRyLmg6 CisgICAgICAgIChXVEY6OldlYWtQdHI6Om9wZXJhdG9yPSk6IEFkZGVkICdudWxscHRyX3QnIG92 ZXJsb2FkLgorICAgICAgICAoV1RGOjpXZWFrUHRyOjpjbGVhcik6IFJlbmFtZWQgZnJvbSAnZm9y Z2V0JworICAgICAgICAoV1RGOjpXZWFrUHRyOjpmb3JnZXQpOiBEZWxldGVkLgorCiAyMDE1LTAy LTIzICBCcmVudCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgogCiAgICAgICAgIFdURjo6 V2Vha1B0ciBzaG91bGQgaGF2ZSBhICdmb3JnZXQnIG1ldGhvZApJbmRleDogU291cmNlL1dURi93 dGYvV2Vha1B0ci5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3RmL1dlYWtQdHIuaAkocmV2 aXNpb24gMTgwNTI4KQorKysgU291cmNlL1dURi93dGYvV2Vha1B0ci5oCSh3b3JraW5nIGNvcHkp CkBAIC0xMDAsMTAgKzEwMCwxMSBAQCBwdWJsaWM6CiAKICAgICBXZWFrUHRyJiBvcGVyYXRvcj0o Y29uc3QgV2Vha1B0ciYgbykgeyBtX3JlZiA9IG8ubV9yZWYuY29weVJlZigpOyByZXR1cm4gKnRo aXM7IH0KICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBVPiBXZWFrUHRyJiBvcGVyYXRvcj0oY29uc3Qg V2Vha1B0cjxVPiYgbykgeyBtX3JlZiA9IG8ubV9yZWYuY29weVJlZigpOyByZXR1cm4gKnRoaXM7 IH0KKyAgICBXZWFrUHRyJiBvcGVyYXRvcj0oc3RkOjpudWxscHRyX3QpIHsgbV9yZWYgPSBXZWFr UmVmZXJlbmNlPFQ+OjpjcmVhdGUobnVsbHB0cik7IHJldHVybiAqdGhpczsgfQogCiAgICAgVCog b3BlcmF0b3ItPigpIGNvbnN0IHsgcmV0dXJuIG1fcmVmLT5nZXQoKTsgfQogCi0gICAgdm9pZCBm b3JnZXQoKSB7IG1fcmVmID0gV2Vha1JlZmVyZW5jZTxUPjo6Y3JlYXRlKG51bGxwdHIpOyB9Cisg ICAgdm9pZCBjbGVhcigpIHsgbV9yZWYgPSBXZWFrUmVmZXJlbmNlPFQ+OjpjcmVhdGUobnVsbHB0 cik7IH0KIAogcHJpdmF0ZToKICAgICBmcmllbmQgY2xhc3MgV2Vha1B0ckZhY3Rvcnk8VD47Cklu ZGV4OiBUb29scy9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvQ2hhbmdlTG9nCShyZXZp c2lvbiAxODA1MzMpCisrKyBUb29scy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyAr MSwxNCBAQAorMjAxNS0wMi0yMyAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGFwcGxlLmNvbT4K KworICAgICAgICBXVEY6OldlYWtQdHIgc2hvdWxkIHJlbmFtZSAnZm9yZ290JyB0byAnY2xlYXIn IGFuZCBzdXBwb3J0IG51bGxwdHIgYXNzaWdubWVudAorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQxOTM1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgKiBUZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9XZWFrUHRy LmNwcDoKKyAgICAgICAgKFRlc3RXZWJLaXRBUEk6OlRFU1QpOiBVcGRhdGVkIGZvciAnY2xlYXIn IG1ldGhvZCByZW5hbWUsIGFuZCBhZGRlZCBhIGZldworICAgICAgICB0ZXN0cyBmb3IgYXNzaWdu aW5nIGZyb20gbnVsbHB0ci4KKwogMjAxNS0wMi0yMyAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFt QGFwcGxlLmNvbT4KIAogICAgICAgICBXVEY6OldlYWtQdHIgc2hvdWxkIGhhdmUgYSAnZm9yZ2V0 JyBtZXRob2QuCkluZGV4OiBUb29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9XZWFrUHRyLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBUb29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9XZWFrUHRyLmNw cAkocmV2aXNpb24gMTgwNTI4KQorKysgVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0cy9XVEYvV2Vh a1B0ci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEyNSwyOSArMTI1LDQxIEBAIFRFU1QoV1RGX1dl YWtQdHIsIEZvcmdldCkKICAgICBpbnQgZHVtbXkyID0gNzsKIAogICAgIFdlYWtQdHJGYWN0b3J5 PGludD4gb3V0ZXJGYWN0b3J5KCZkdW1teTIpOwotICAgIFdlYWtQdHI8aW50PiB3ZWFrUHRyMSwg d2Vha1B0cjIsIHdlYWtQdHI0OworICAgIFdlYWtQdHI8aW50PiB3ZWFrUHRyMSwgd2Vha1B0cjIs IHdlYWtQdHIzLCB3ZWFrUHRyNDsKICAgICB7CiAgICAgICAgIFdlYWtQdHJGYWN0b3J5PGludD4g aW5uZXJGYWN0b3J5KCZkdW1teSk7CiAgICAgICAgIHdlYWtQdHIxID0gaW5uZXJGYWN0b3J5LmNy ZWF0ZVdlYWtQdHIoKTsKICAgICAgICAgd2Vha1B0cjIgPSBpbm5lckZhY3RvcnkuY3JlYXRlV2Vh a1B0cigpOworICAgICAgICB3ZWFrUHRyMyA9IGlubmVyRmFjdG9yeS5jcmVhdGVXZWFrUHRyKCk7 CiAgICAgICAgIEVYUEVDVF9FUSh3ZWFrUHRyMS5nZXQoKSwgJmR1bW15KTsKICAgICAgICAgRVhQ RUNUX0VRKHdlYWtQdHIyLmdldCgpLCAmZHVtbXkpOwotICAgICAgICB3ZWFrUHRyMS5mb3JnZXQo KTsKKyAgICAgICAgRVhQRUNUX0VRKHdlYWtQdHIzLmdldCgpLCAmZHVtbXkpOworICAgICAgICB3 ZWFrUHRyMS5jbGVhcigpOworICAgICAgICB3ZWFrUHRyMyA9IG51bGxwdHI7CiAgICAgICAgIEVY UEVDVF9OVUxMKHdlYWtQdHIxLmdldCgpKTsKICAgICAgICAgRVhQRUNUX0VRKHdlYWtQdHIyLmdl dCgpLCAmZHVtbXkpOwotICAgICAgICB3ZWFrUHRyMS5mb3JnZXQoKTsKKyAgICAgICAgRVhQRUNU X05VTEwod2Vha1B0cjMuZ2V0KCkpOworICAgICAgICB3ZWFrUHRyMS5jbGVhcigpOworICAgICAg ICB3ZWFrUHRyMy5jbGVhcigpOwogICAgICAgICBFWFBFQ1RfTlVMTCh3ZWFrUHRyMS5nZXQoKSk7 CiAgICAgICAgIEVYUEVDVF9FUSh3ZWFrUHRyMi5nZXQoKSwgJmR1bW15KTsKLSAgICAgICAgV2Vh a1B0cjxpbnQ+IHdlYWtQdHIzID0gd2Vha1B0cjI7Ci0gICAgICAgIEVYUEVDVF9FUSh3ZWFrUHRy Mi5nZXQoKSwgJmR1bW15KTsKLSAgICAgICAgRVhQRUNUX0VRKHdlYWtQdHIzLmdldCgpLCAmZHVt bXkpOwotICAgICAgICB3ZWFrUHRyMy5mb3JnZXQoKTsKICAgICAgICAgRVhQRUNUX05VTEwod2Vh a1B0cjMuZ2V0KCkpOworICAgICAgICB3ZWFrUHRyMyA9IG51bGxwdHI7CisgICAgICAgIEVYUEVD VF9OVUxMKHdlYWtQdHIxLmdldCgpKTsKICAgICAgICAgRVhQRUNUX0VRKHdlYWtQdHIyLmdldCgp LCAmZHVtbXkpOworICAgICAgICBFWFBFQ1RfTlVMTCh3ZWFrUHRyMy5nZXQoKSk7CisgICAgICAg IAogICAgICAgICB3ZWFrUHRyNCA9IHdlYWtQdHIyOwogICAgICAgICBFWFBFQ1RfRVEod2Vha1B0 cjIuZ2V0KCksICZkdW1teSk7CiAgICAgICAgIEVYUEVDVF9FUSh3ZWFrUHRyNC5nZXQoKSwgJmR1 bW15KTsKIAorICAgICAgICBXZWFrUHRyPGludD4gd2Vha1B0cjUgPSB3ZWFrUHRyMjsKKyAgICAg ICAgRVhQRUNUX0VRKHdlYWtQdHIyLmdldCgpLCAmZHVtbXkpOworICAgICAgICBFWFBFQ1RfRVEo d2Vha1B0cjUuZ2V0KCksICZkdW1teSk7CisgICAgICAgIHdlYWtQdHI1LmNsZWFyKCk7CisgICAg ICAgIEVYUEVDVF9OVUxMKHdlYWtQdHI1LmdldCgpKTsKKyAgICAgICAgRVhQRUNUX0VRKHdlYWtQ dHIyLmdldCgpLCAmZHVtbXkpOworCiAgICAgICAgIHdlYWtQdHI0ID0gb3V0ZXJGYWN0b3J5LmNy ZWF0ZVdlYWtQdHIoKTsKICAgICAgICAgRVhQRUNUX0VRKHdlYWtQdHIyLmdldCgpLCAmZHVtbXkp OwogICAgICAgICBFWFBFQ1RfRVEod2Vha1B0cjQuZ2V0KCksICZkdW1teTIpOwpAQCAtMTYwLDEx ICsxNzIsMTYgQEAgVEVTVChXVEZfV2Vha1B0ciwgRm9yZ2V0KQogICAgIFdlYWtQdHI8aW50PiB3 ZWFrUHRyNSA9IHdlYWtQdHI0OwogICAgIEVYUEVDVF9FUSh3ZWFrUHRyNC5nZXQoKSwgJmR1bW15 Mik7CiAgICAgRVhQRUNUX0VRKHdlYWtQdHI1LmdldCgpLCAmZHVtbXkyKTsKLSAgICB3ZWFrUHRy NS5mb3JnZXQoKTsKKyAgICB3ZWFrUHRyNS5jbGVhcigpOwogICAgIEVYUEVDVF9OVUxMKHdlYWtQ dHI1LmdldCgpKTsKICAgICBXZWFrUHRyPGludD4gd2Vha1B0cjYgPSB3ZWFrUHRyNTsKICAgICBF WFBFQ1RfTlVMTCh3ZWFrUHRyNi5nZXQoKSk7CiAgICAgRVhQRUNUX0VRKHdlYWtQdHI1LmdldCgp LCB3ZWFrUHRyNi5nZXQoKSk7CisKKyAgICBXZWFrUHRyPGludD4gd2Vha1B0cjcgPSBvdXRlckZh Y3RvcnkuY3JlYXRlV2Vha1B0cigpOworICAgIEVYUEVDVF9FUSh3ZWFrUHRyNy5nZXQoKSwgJmR1 bW15Mik7CisgICAgd2Vha1B0cjcgPSBudWxscHRyOworICAgIEVYUEVDVF9OVUxMKHdlYWtQdHI3 LmdldCgpKTsKIH0KICAgICAKIH0gLy8gbmFtZXNwYWNlIFRlc3RXZWJLaXRBUEkK