66024 2011-08-10 17:09:50 -0700 nullptr can't be used for PassRefPtr 2011-08-11 19:39:33 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 66123 1 jamesr jamesr andersca bashi darin eric webkit.review.bot oldest_to_newest 449505 0 jamesr 2011-08-10 17:09:50 -0700 nullptr can't be used for PassRefPtr 449506 1 103561 jamesr 2011-08-10 17:10:12 -0700 Created attachment 103561 Patch 449508 2 jamesr 2011-08-10 17:12:45 -0700 When a function defined to return PassRefPtr<T> wants to return a null value, it currently has to do either: return 0; which isn't as descriptive as it could be or return PassRefPtr<T>(); which can be a lot of typing if "T" is long. It'd be nice to be able to do return nullptr; like you can for PassOwnPtr<>. It looks like this functionality was added by Darin back in http://trac.webkit.org/changeset/69970 but then deleted by Anders in http://trac.webkit.org/changeset/89283. I think the nullptr constructor was just an innocent victim in http://trac.webkit.org/changeset/89283 - it seems like something that we do want to support. 449549 3 andersca 2011-08-10 18:33:29 -0700 (In reply to comment #2) > When a function defined to return PassRefPtr<T> wants to return a null value, it currently has to do either: > > return 0; > > which isn't as descriptive as it could be or > > return PassRefPtr<T>(); > > which can be a lot of typing if "T" is long. It'd be nice to be able to do > > return nullptr; > > like you can for PassOwnPtr<>. > Yeah, I agree. > It looks like this functionality was added by Darin back in http://trac.webkit.org/changeset/69970 but then deleted by Anders in http://trac.webkit.org/changeset/89283. I think the nullptr constructor was just an innocent victim in http://trac.webkit.org/changeset/89283 - it seems like something that we do want to support. Looking at the commit, it looks like only the nullptr assignment operator was removed and that there was never a nullptr constructor (unless I'm missing something). 449973 4 jamesr 2011-08-11 13:13:09 -0700 (In reply to comment #3) > (In reply to comment #2) > > When a function defined to return PassRefPtr<T> wants to return a null value, it currently has to do either: > > > > return 0; > > > > which isn't as descriptive as it could be or > > > > return PassRefPtr<T>(); > > > > which can be a lot of typing if "T" is long. It'd be nice to be able to do > > > > return nullptr; > > > > like you can for PassOwnPtr<>. > > > > Yeah, I agree. > > > It looks like this functionality was added by Darin back in http://trac.webkit.org/changeset/69970 but then deleted by Anders in http://trac.webkit.org/changeset/89283. I think the nullptr constructor was just an innocent victim in http://trac.webkit.org/changeset/89283 - it seems like something that we do want to support. > > Looking at the commit, it looks like only the nullptr assignment operator was removed and that there was never a nullptr constructor (unless I'm missing something). Aha, you are correct. Still, I think this patch is good. 449981 5 andersca 2011-08-11 13:17:11 -0700 (In reply to comment #4) > Aha, you are correct. > > Still, I think this patch is good. I agree. I'm not sure if it'll break the C++0x build, but seeing as we don't have a C++0x build, I don't think we need to worry about that now. 450043 6 103561 webkit.review.bot 2011-08-11 14:40:04 -0700 Comment on attachment 103561 Patch Clearing flags on attachment: 103561 Committed r92880: <http://trac.webkit.org/changeset/92880> 450044 7 webkit.review.bot 2011-08-11 14:40:08 -0700 All reviewed patches have been landed. Closing bug. 450222 8 bashi 2011-08-11 18:47:37 -0700 Hi, This looks great, but the change causes a bunch of compile error when I build Chromium Win port with VS2010. How can I fix the error? 450223 9 jamesr 2011-08-11 18:48:56 -0700 Can you show me a link to a failing build or post the compile errors here? 450227 10 bashi 2011-08-11 18:56:51 -0700 (In reply to comment #9) > Can you show me a link to a failing build or post the compile errors here? My build environment is Japanese so you might not get the error description, but the error code is C2668. http://msdn.microsoft.com/en-us/library/da60x087(v=VS.100).aspx c:\cygwin\home\bashi\chrome\src\third_party\webkit\source\javascriptcore\wtf\text\StringImpl.h(173): error C2668: 'WTF::PassRefPtr<T>::PassRefPtr' : オーバーロード関数の呼び出しを解決することができません。(新機能 ; ヘルプを参照) 3> with 3> [ 3> T=WTF::StringImpl 3> ] 450230 11 jamesr 2011-08-11 19:07:13 -0700 (In reply to comment #10) > (In reply to comment #9) > > Can you show me a link to a failing build or post the compile errors here? > > My build environment is Japanese so you might not get the error description, but the error code is C2668. > http://msdn.microsoft.com/en-us/library/da60x087(v=VS.100).aspx > > c:\cygwin\home\bashi\chrome\src\third_party\webkit\source\javascriptcore\wtf\text\StringImpl.h(173): error C2668: 'WTF::PassRefPtr<T>::PassRefPtr' : オーバーロード関数の呼び出しを解決することができません。(新機能 ; ヘルプを参照) > 3> with > 3> [ > 3> T=WTF::StringImpl > 3> ] Aha, this looks interesting (from the msdn page): "You can also get this error through template use. If, in the same class, you have a regular member function and a templated member function with the same signature, the templated one must come first. This is a limitation of the current implementation of Visual C++." Can you try swapping these two lines: PassRefPtr(std::nullptr_t) : m_ptr(0) { } PassRefPtr(T* ptr) : m_ptr(ptr) { refIfNotNull(ptr); } and see if that fixes the issue for you? 450241 12 jamesr 2011-08-11 19:37:27 -0700 So the problem is we have: foo(void*) {} foo(nullptr) {} and VS2010 can't figure out what we mean when we do: foo(0); That's unfortunate. I don't know any way to resolve this other than replacing all the code that does constructs a PassRefPtr<> with the literal 0 by nullptr, which will kind of suck. 103561 2011-08-10 17:10:12 -0700 2011-08-11 14:40:04 -0700 Patch bug-66024-20110810171010.patch text/plain 1382 jamesr U3VidmVyc2lvbiBSZXZpc2lvbjogOTI3OTkKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDI0 MGI1MmE0MDU5MzQ0NTVjMmM5NGNhZGQwZTNjNWJjNzg3MmZlZTEuLjE4NGUxNzAyNzRkNDdkYzA2 ZDEzYTVlN2ZkOGRmMTRhNDM3YWEwNjYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDEzIEBACisyMDExLTA4LTEwICBKYW1lcyBSb2JpbnNvbiAgPGphbWVzckBjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgbnVsbHB0ciBjYW4ndCBiZSB1c2VkIGZvciBQYXNzUmVmUHRyCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NjAyNAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL1Bhc3NSZWZQ dHIuaDoKKyAgICAgICAgKFdURjo6UGFzc1JlZlB0cjo6UGFzc1JlZlB0cik6CisKIDIwMTEtMDgt MTAgIE1hcmsgSGFobmVuYmVyZyAgPG1oYWhuZW5iZXJnQGFwcGxlLmNvbT4KIAogICAgICAgICBK U0Mgc2hvdWxkIGFsd2F5cyB0aHJvdyB3aGVuIGZ1bmN0aW9uIGFyZyBsaXN0IGlzIHRvbyBsb25n CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL1Bhc3NSZWZQdHIuaCBiL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvUGFzc1JlZlB0ci5oCmluZGV4IGEzNGE2ZmFkMGVhM2Qw MDI3YjE3MmUxODQ2ZGJmZjZiYTlhZGIyZTQuLjA4NDJkOWE3NGJiNDkzNTM4NmYxYzgzNWFjZTJk MTZmMTZhM2MwOWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvUGFzc1Jl ZlB0ci5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvUGFzc1JlZlB0ci5oCkBAIC02 NCw2ICs2NCw3IEBAIG5hbWVzcGFjZSBXVEYgewogICAgIHRlbXBsYXRlPHR5cGVuYW1lIFQ+IGNs YXNzIFBhc3NSZWZQdHIgewogICAgIHB1YmxpYzoKICAgICAgICAgUGFzc1JlZlB0cigpIDogbV9w dHIoMCkgeyB9CisgICAgICAgIFBhc3NSZWZQdHIoc3RkOjpudWxscHRyX3QpIDogbV9wdHIoMCkg eyB9CiAgICAgICAgIFBhc3NSZWZQdHIoVCogcHRyKSA6IG1fcHRyKHB0cikgeyByZWZJZk5vdE51 bGwocHRyKTsgfQogICAgICAgICAvLyBJdCBzb21ld2hhdCBicmVha3MgdGhlIHR5cGUgc3lzdGVt IHRvIGFsbG93IHRyYW5zZmVyIG9mIG93bmVyc2hpcCBvdXQgb2YKICAgICAgICAgLy8gYSBjb25z dCBQYXNzUmVmUHRyLiBIb3dldmVyLCBpdCBtYWtlcyBpdCBtdWNoIGVhc2llciB0byB3b3JrIHdp dGggUGFzc1JlZlB0cgo=