74159 2011-12-08 20:18:13 -0800 Fix HashMap<..., OwnPtr<...> >::add compilation errors 2011-12-09 10:59:32 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 74154 1 enne enne darin enne jamesr oldest_to_newest 518066 0 enne 2011-12-08 20:18:13 -0800 Fix HashMap<..., OwnPtr<...> >::add compilation errors 518073 1 118521 enne 2011-12-08 20:34:12 -0800 Created attachment 118521 Patch 518075 2 enne 2011-12-08 20:37:21 -0800 When I saw Adam's fix for bug 73964, I thought I'd take advantage of using an OwnPtr as the value for a HashMap in CCLayerTilingData. When I did this (see patch in bug 74154), I got the following compilation error: /usr/include/c++/4.4/bits/stl_pair.h: In constructor 'std::pair<_T1, _T2>::pair(const std::pair<_U1, _U2>&) [with _U1 = std::pair<int, int>, _U2 = std::nullptr_t, _T1 = std::pair<int, int>, _T2 = WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>]': /usr/include/c++/4.4/bits/stl_pair.h:101: error: no matching function for call to 'WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>::OwnPtr(const std::nullptr_t&)' So, I added this missing function to OwnPtr. 518336 3 118521 darin 2011-12-09 10:37:40 -0800 Comment on attachment 118521 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118521&action=review > Source/JavaScriptCore/wtf/OwnPtr.h:44 > + explicit OwnPtr(const std::nullptr_t&) : m_ptr(0) { } I see no reason for this to be explicit. This might work for some compilers, but with others I fear it may break the build. The EWS seems to like it OK. Since you did not include the entire error, I cannot be sure, but I suspect there may be a way to resolve this entirely in HashTraits.h. For example, it may be a simple matter of overloading some function to take a nullptr_t. I am not going to say review+ yet because of the “explicit” issue and the fact that I’d like to see the rest of the error. 518337 4 118521 darin 2011-12-09 10:40:17 -0800 Comment on attachment 118521 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118521&action=review r=me, but please land my revised version >> Source/JavaScriptCore/wtf/OwnPtr.h:44 >> + explicit OwnPtr(const std::nullptr_t&) : m_ptr(0) { } > > I see no reason for this to be explicit. > > This might work for some compilers, but with others I fear it may break the build. The EWS seems to like it OK. > > Since you did not include the entire error, I cannot be sure, but I suspect there may be a way to resolve this entirely in HashTraits.h. For example, it may be a simple matter of overloading some function to take a nullptr_t. > > I am not going to say review+ yet because of the “explicit” issue and the fact that I’d like to see the rest of the error. I changed my mind about all the rest of my comment. This is a good change. But we should not use “explicit” here. It’s good and helpful to allow implicit conversion from nullptr_t to OwnPtr. It should read: OwnPtr(std::nullptr_t) : m_ptr(0) { } There is no reason to use const std::nullptr_t& instead of just std::nullptr_t. See PassOwnPtr, which already has this constructor. 518340 5 enne 2011-12-09 10:44:53 -0800 (In reply to comment #4) > > It should read: > > OwnPtr(std::nullptr_t) : m_ptr(0) { } > > There is no reason to use const std::nullptr_t& instead of just std::nullptr_t. See PassOwnPtr, which already has this constructor. Thanks--both of those changes make a lot of sense. I'll land it exactly as above. 518347 6 enne 2011-12-09 10:59:32 -0800 Committed r102459: <http://trac.webkit.org/changeset/102459> 118521 2011-12-08 20:34:12 -0800 2011-12-09 10:40:17 -0800 Patch bug-74159-20111208203411.patch text/plain 1392 enne U3VidmVyc2lvbiBSZXZpc2lvbjogMTAyNDExCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh ZGE5OTQwMTNlOWYwMDIwNzY3N2Y1YWE1YWRmOGUxYWEwMjQ2MTRhLi4yYzFiNmU5MmU1YmM1MjU0 NjM0YWIyYmMwZWFkMjY5ZjQyOTljZGZhIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxMS0xMi0wOCAgQWRyaWVubmUgV2Fsa2VyICA8ZW5uZUBnb29nbGUuY29t PgorCisgICAgICAgIEZpeCBIYXNoTWFwPC4uLiwgT3duUHRyPC4uLj4gPjo6YWRkIGNvbXBpbGF0 aW9uIGVycm9ycworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NzQxNTkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBBZGQgYSBjb25zdHJ1Y3RvciB0byBPd25QdHIgdGhhdCB0YWtlcyB0aGUgZW1wdHkgdmFsdWUg KG51bGxwdHJfdCkKKyAgICAgICAgZnJvbSBIYXNoVHJhaXRzIHNvIHRoYXQgdGhpcyBmdW5jdGlv biBjYW4gY29tcGlsZS4KKworICAgICAgICAqIHd0Zi9Pd25QdHIuaDoKKyAgICAgICAgKFdURjo6 T3duUHRyOjpPd25QdHIpOgorCiAyMDExLTEyLTA4ICBBZGFtIEtsZWluICA8YWRhbWtAY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFVzZSBIYXNoTWFwPE5vZGUqLCBPd25QdHI8Li4uPj4gaW4gQ2hp bGRMaXN0TXV0YXRpb25TY29wZQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0 Zi9Pd25QdHIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvT3duUHRyLmgKaW5kZXggMzEx NDM1NWE4MGE5YjhhOGNiY2QwNTg3ZGU2NDc4MzExNzYwNzJkYS4uYzNlZjQ0ZmRjOWY4ZTczYmE3 YzRhMTg5NmNlYjQ0NDFhZWM1YTViNyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L3d0Zi9Pd25QdHIuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL093blB0ci5oCkBA IC00MSw2ICs0MSw3IEBAIG5hbWVzcGFjZSBXVEYgewogICAgICAgICB0eXBlZGVmIFZhbHVlVHlw ZSogUHRyVHlwZTsKIAogICAgICAgICBPd25QdHIoKSA6IG1fcHRyKDApIHsgfQorICAgICAgICBl eHBsaWNpdCBPd25QdHIoY29uc3Qgc3RkOjpudWxscHRyX3QmKSA6IG1fcHRyKDApIHsgfQogCiAg ICAgICAgIC8vIFNlZSBjb21tZW50IGluIFBhc3NPd25QdHIuaCBmb3Igd2h5IHRoaXMgdGFrZXMg YSBjb25zdCByZWZlcmVuY2UuCiAgICAgICAgIHRlbXBsYXRlPHR5cGVuYW1lIFU+IE93blB0cihj b25zdCBQYXNzT3duUHRyPFU+JiBvKTsK