Bug 66024 - nullptr can't be used for PassRefPtr
Summary: nullptr can't be used for PassRefPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 66123
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-10 17:09 PDT by James Robinson
Modified: 2011-08-11 19:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.35 KB, patch)
2011-08-10 17:10 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-08-10 17:09:50 PDT
nullptr can't be used for PassRefPtr
Comment 1 James Robinson 2011-08-10 17:10:12 PDT
Created attachment 103561 [details]
Patch
Comment 2 James Robinson 2011-08-10 17:12:45 PDT
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.
Comment 3 Anders Carlsson 2011-08-10 18:33:29 PDT
(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).
Comment 4 James Robinson 2011-08-11 13:13:09 PDT
(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.
Comment 5 Anders Carlsson 2011-08-11 13:17:11 PDT
(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.
Comment 6 WebKit Review Bot 2011-08-11 14:40:04 PDT
Comment on attachment 103561 [details]
Patch

Clearing flags on attachment: 103561

Committed r92880: <http://trac.webkit.org/changeset/92880>
Comment 7 WebKit Review Bot 2011-08-11 14:40:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Kenichi Ishibashi 2011-08-11 18:47:37 PDT
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?
Comment 9 James Robinson 2011-08-11 18:48:56 PDT
Can you show me a link to a failing build or post the compile errors here?
Comment 10 Kenichi Ishibashi 2011-08-11 18:56:51 PDT
(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>          ]
Comment 11 James Robinson 2011-08-11 19:07:13 PDT
(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?
Comment 12 James Robinson 2011-08-11 19:37:27 PDT
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.