Remove implicit conversions from scoped_refptr to T* in mojo/

Remove implicit conversions from scoped_refptr to T* in mojo/

This patch was generated by running the rewrite_scoped_refptr clang tool
on a Linux build.

BUG=110610
Committed: https://crrev.com/f28421761b5b19f33c011b3ca016bdb88b858c63
Cr-Commit-Position: refs/heads/master@{#291766}

[viettrungluu, 2014-08-25 19:57:05]

I think this change is all wrong. scoped_refptr should be testable.

https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc
File mojo/system/channel.cc (right):

https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
mojo/system/channel.cc:91: DCHECK(!it->second.message_pipe.get());
Eh? This is a truth (i.e., (non)null) check. Which shouldn't get converted, no?

https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
mojo/system/channel.cc:108: DCHECK(message_pipe.get());
"

[dcheng, 2014-08-25 19:58:29]

https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc
File mojo/system/channel.cc (right):

https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
mojo/system/channel.cc:91: DCHECK(!it->second.message_pipe.get());
On 2014/08/25 19:57:05, viettrungluu wrote:
> Eh? This is a truth (i.e., (non)null) check. Which shouldn't get converted,
no?

I can't add a boolean test to scoped_refptr until all implicit conversions to T*
have been removed on a platform. The conversion would be ambiguous otherwise.

[viettrungluu, 2014-08-25 20:09:49]

On 2014/08/25 19:58:29, dcheng (OOO) wrote:
> https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc
> File mojo/system/channel.cc (right):
> 
>
https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
> mojo/system/channel.cc:91: DCHECK(!it->second.message_pipe.get());
> On 2014/08/25 19:57:05, viettrungluu wrote:
> > Eh? This is a truth (i.e., (non)null) check. Which shouldn't get converted,
> no?
> 
> I can't add a boolean test to scoped_refptr until all implicit conversions to
T*
> have been removed on a platform. The conversion would be ambiguous otherwise.

I'm not sure I follow the logic behind your conversion process.

If you were to make scoped_refptr testable but not implicitly convertible, 100%
of the errors you'd get would indicate a need for .get().

If you simply remove implicit conversions and add .get()s everywhere, and later
make it testable, then you'll need to go back and figure out which ones
shouldn't have .get().

[dcheng, 2014-08-25 20:18:16]

On 2014/08/25 at 20:09:49, viettrungluu wrote:
> On 2014/08/25 19:58:29, dcheng (OOO) wrote:
> > https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc
> > File mojo/system/channel.cc (right):
> > 
> >
https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
> > mojo/system/channel.cc:91: DCHECK(!it->second.message_pipe.get());
> > On 2014/08/25 19:57:05, viettrungluu wrote:
> > > Eh? This is a truth (i.e., (non)null) check. Which shouldn't get
converted,
> > no?
> > 
> > I can't add a boolean test to scoped_refptr until all implicit conversions
to T*
> > have been removed on a platform. The conversion would be ambiguous
otherwise.
> 
> I'm not sure I follow the logic behind your conversion process.
> 
> If you were to make scoped_refptr testable but not implicitly convertible,
100% of the errors you'd get would indicate a need for .get().
> 
> If you simply remove implicit conversions and add .get()s everywhere, and
later make it testable, then you'll need to go back and figure out which ones
shouldn't have .get().

Sorry for the copy paste (there will be some redundancy), but I'm including this
blurb in new reviews I'm sending out:
====
Unfortunately, we can't make scoped_refptr testable until the conversion
operator has been removed. While it would be possible to disable the conversion
operator and make it testable in the same patch, the automated tool tries to
skip rewriting code where manual cleanups would be useful. This includes things
like:

// What if ReturnsScopedRefPtr() creates a new object internally?
if (ReturnsScopedRefPtr())

Foo* Bar() {
  const scoped_refptr<Foo>& f = ReturnsScopedRefPtr();
  return f;  // Does the return type need to be rewritten? Or is .get() safe?
}

Doing this in one patch would make catching some of these instances (in
particular, a boolean test on a temporary scoped_refptr) more difficult. As a
result, the work is being split into several steps:
Step 1: Rewrite all implicit conversions to be explicit (or rewrite the receiver
type to scoped_refptr<T>, if the tool believes the conversion to be unsafe).
Step 2: Once a platform compiles cleanly without the conversion operator, #ifdef
the conversion operator out.
Step 3: Make scoped_refptr testable on that platform.
Step 4: Remove unnecessary calls to .get().

[viettrungluu, 2014-08-25 20:38:47]

On 2014/08/25 20:18:16, dcheng (OOO) wrote:
> On 2014/08/25 at 20:09:49, viettrungluu wrote:
> > On 2014/08/25 19:58:29, dcheng (OOO) wrote:
> > > https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc
> > > File mojo/system/channel.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/502573006/diff/1/mojo/system/channel.cc#newco...
> > > mojo/system/channel.cc:91: DCHECK(!it->second.message_pipe.get());
> > > On 2014/08/25 19:57:05, viettrungluu wrote:
> > > > Eh? This is a truth (i.e., (non)null) check. Which shouldn't get
> converted,
> > > no?
> > > 
> > > I can't add a boolean test to scoped_refptr until all implicit conversions
> to T*
> > > have been removed on a platform. The conversion would be ambiguous
> otherwise.
> > 
> > I'm not sure I follow the logic behind your conversion process.
> > 
> > If you were to make scoped_refptr testable but not implicitly convertible,
> 100% of the errors you'd get would indicate a need for .get().
> > 
> > If you simply remove implicit conversions and add .get()s everywhere, and
> later make it testable, then you'll need to go back and figure out which ones
> shouldn't have .get().
> 
> Sorry for the copy paste (there will be some redundancy), but I'm including
this
> blurb in new reviews I'm sending out:
> ====
> Unfortunately, we can't make scoped_refptr testable until the conversion
> operator has been removed. While it would be possible to disable the
conversion
> operator and make it testable in the same patch, the automated tool tries to
> skip rewriting code where manual cleanups would be useful. This includes
things
> like:
> 
> // What if ReturnsScopedRefPtr() creates a new object internally?
> if (ReturnsScopedRefPtr())
> 
> Foo* Bar() {
>   const scoped_refptr<Foo>& f = ReturnsScopedRefPtr();
>   return f;  // Does the return type need to be rewritten? Or is .get() safe?
> }
> 
> Doing this in one patch would make catching some of these instances (in
> particular, a boolean test on a temporary scoped_refptr) more difficult. As a
> result, the work is being split into several steps:
> Step 1: Rewrite all implicit conversions to be explicit (or rewrite the
receiver
> type to scoped_refptr<T>, if the tool believes the conversion to be unsafe).
> Step 2: Once a platform compiles cleanly without the conversion operator,
#ifdef
> the conversion operator out.
> Step 3: Make scoped_refptr testable on that platform.
> Step 4: Remove unnecessary calls to .get().

I don't really know why you don't just combine Steps 2 and 3 (and in Step 1 only
rewrite the conversions that need to be converted to make 2+3 happen), and avoid
the churn. (You could still do Step 4 as-is, to remove existing .get()s in
boolean checks.)

But okay, LGTM.

[dcheng, 2014-08-25 21:07:39]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-25 21:09:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/502573006/1

[commit-bot: I haz the power, 2014-08-25 21:57:57]

Committed patchset #1 (1) as 6cfdecfe69a1ae2a6062a74c72c4a83c62b9c74a

[commit-bot: I haz the power, 2014-09-10 02:37:46]

Patchset 1 (id:??) landed as
https://crrev.com/f28421761b5b19f33c011b3ca016bdb88b858c63
Cr-Commit-Position: refs/heads/master@{#291766}